Re: [PATCH v5 5/5] iommufd/vdevice: add TSM request ioctl

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The ioctl handler incorrectly truncates the `ssize_t` residue from `tsm_guest_req()` to a 32-bit `int`, causing valid large positive residues to be misinterpreted as negative error codes.
> - [Medium] The `IOMMU_VDEVICE_TSM_REQ` ioctl omits the TSM implementation-specific result code (`tsm_code`), preventing userspace from forwarding it to the guest.
> - [Low] Manual initialization of `sockptr_t` bypasses kernel macros.
> --
>
> commit d5ee59b85809ac88694767fcc899de1ccc2821ca
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> iommufd/vdevice: add TSM request ioctl
>
> This commit adds the IOMMU_VDEVICE_TSM_REQ ioctl to issue TSM guest
> request/response transactions against an iommufd vdevice. It forwards
> these requests through the new tsm_guest_req() helper to the PCI TSM
> backend, supporting CoCo guest device attestation.
>
>> diff --git a/drivers/iommu/iommufd/tsm.c b/drivers/iommu/iommufd/tsm.c
>> index 09ee668dbed9d..342fbdb6a6b95 100644
>> --- a/drivers/iommu/iommufd/tsm.c
>> +++ b/drivers/iommu/iommufd/tsm.c
> [ ... ]
>> +int iommufd_vdevice_tsm_req_ioctl(struct iommufd_ucmd *ucmd)
>> +{
>> +	int rc;
>
> [Severity: High]
> tsm_guest_req() returns an ssize_t representing the unconsumed byte residue,
> and the user-provided req_len and resp_len are __u32.
>
> If the residue is between 2GB and 4GB, could it overflow this 32-bit signed
> rc variable and become a negative value? This could cause the libc ioctl()
> wrapper to incorrectly treat a successful request as a failure.
>
> Would it be safer to return the residue by modifying a field in
> struct iommu_vdevice_tsm_req instead of returning unbounded positive values
> via the ioctl return code?
>

req_len and resp_len are both 32 bits.

>
>> +	struct iommufd_vdevice *vdev;
>> +	struct iommu_vdevice_tsm_req *cmd = ucmd->cmd;
>> +	struct tsm_guest_req_info info = {
>> +		.scope = cmd->scope,
>> +		.req   = {
>> +			.user = u64_to_user_ptr(cmd->req_uptr),
>> +			.is_kernel = false,
>> +		},
>
> [Severity: Low]
> Could this use the USER_SOCKPTR() macro to initialize the sockptr_t safely
> instead of manually setting .user and .is_kernel? Bypassing the macro makes
> this fragile against future layout changes in the sockptr_t structure.
>
>> +		.req_len = cmd->req_len,
>> +		.resp    =  {
>> +			.user = u64_to_user_ptr(cmd->resp_uptr),
>> +			.is_kernel = false,
>> +		},
>> +		.resp_len = cmd->resp_len,
>> +	};
> [ ... ]
>> diff --git a/drivers/virt/coco/tsm-core.c b/drivers/virt/coco/tsm-core.c
>> index 3870d08ffe0d1..c24886851f9e0 100644
>> --- a/drivers/virt/coco/tsm-core.c
>> +++ b/drivers/virt/coco/tsm-core.c
> [ ... ]
>> +ssize_t tsm_guest_req(struct device *dev, struct tsm_guest_req_info *info)
>> +{
>> +	int ret;
>> +	enum pci_tsm_req_scope pci_scope;
>> +
>> +	if (!dev_is_pci(dev))
>> +		return -EINVAL;
>> +
>> +	ret = tsm_pci_req_scope(info->scope, &pci_scope);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return pci_tsm_guest_req(to_pci_dev(dev), pci_scope, info->req,
>> +				 info->req_len, info->resp, info->resp_len,
>> +				 NULL);
>
> [Severity: Medium]
> pci_tsm_guest_req() supports returning a TSM implementation-specific result
> code via the tsm_code parameter, but it is passed NULL here and omitted
> from struct iommu_vdevice_tsm_req.
>
> Do userspace VMMs need this architecture-specific result code to
> determine the outcome and failure reasons of attestation flows, and
> accurately emulate the hardware TSM interface for the guest?
>

Will update the iotcl to include tsm_code as part of the response. 

>
>> +}
>> +EXPORT_SYMBOL_GPL(tsm_guest_req);
>

-aneesh