On Tue, Aug 15, 2023 at 10:31:09AM -0700, Nicolin Chen wrote:
On Tue, Aug 15, 2023 at 01:32:01PM -0300, Jason Gunthorpe wrote:
On Fri, Aug 11, 2023 at 12:15:00AM -0700, Yi Liu wrote:
+static int iommufd_get_hw_info(struct iommufd_ucmd *ucmd) +{
- struct iommu_hw_info *cmd = ucmd->cmd;
- unsigned int length = cmd->data_len;
- struct iommufd_device *idev;
- void __user *user_ptr;
- u32 hw_info_type;
- int rc = 0;
- if (cmd->flags || cmd->__reserved || !cmd->data_len)
return -EOPNOTSUPP;Is there a reason to block 0 data_len? I think this should work. The code looks OK?
I did a quick test passing !data_len and !data_ptr. And it works by returning the type only.
Yet, in that case, should we mention this in the uAPI kdoc? It feels to me that the uAPI always expects user space to read out a length of data.
Well the way it ought to work is that userspace can pass in 0 length and the kernel will return the correct length
So maybe this does need resending with this removed:
*length = min(*length, data_len);
Also I see clear_user is called wrong, it doesn't return errno.
Please check and repost it ASAP I will update the branch. Probably needs some doc adjusting too.
I came up with this:
int iommufd_get_hw_info(struct iommufd_ucmd *ucmd) { struct iommu_hw_info *cmd = ucmd->cmd; void __user *user_ptr = u64_to_user_ptr(cmd->data_ptr); const struct iommu_ops *ops; struct iommufd_device *idev; unsigned int data_len; unsigned int copy_len; void *data = NULL; int rc;
if (cmd->flags || cmd->__reserved) return -EOPNOTSUPP;
idev = iommufd_get_device(ucmd, cmd->dev_id); if (IS_ERR(idev)) return PTR_ERR(idev);
ops = dev_iommu_ops(idev->dev); if (!ops->hw_info) { data = ops->hw_info(idev->dev, &data_len, &cmd->out_data_type); if (IS_ERR(data)) { rc = PTR_ERR(data); goto err_put; }
/* * drivers that have hw_info callback should have a unique * iommu_hw_info_type. */ if (WARN_ON_ONCE(cmd->out_data_type == IOMMU_HW_INFO_TYPE_NONE)) { rc = -ENODEV; goto out; } } else { cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE; data_len = 0; data = NULL; }
copy_len = min(cmd->data_len, data_len); if (copy_to_user(user_ptr, data, copy_len)) { rc = -EFAULT; goto out; }
/* * Zero the trailing bytes if the user buffer is bigger than the * data size kernel actually has. */ if (copy_len < cmd->data_len) { if (clear_user(user_ptr + copy_len, cmd->data_len - copy_len)) { rc = -EFAULT; goto out; } }
/* * We return the length the kernel supports so userspace may know what * the kernel capability is. It could be larger than the input buffer. */ cmd->data_len = data_len;
rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); out: kfree(data); err_put: iommufd_put_object(&idev->obj); return rc; }