Re: [PATCH v2] virtio-iommu: Fix the partial copy of probe request

[Eric Auger <eric.auger@redhat.com>]

On 6/22/22 13:55, Jean-Philippe Brucker wrote:
> Hi,
>
> On Wed, Jun 22, 2022 at 12:20:45PM +0200, Eric Auger wrote:
>> Hi,
>>
>> On 6/17/22 08:20, Zhenzhong Duan wrote:
>>> The structure of probe request doesn't include the tail, this leads
>>> to a few field missed to be copied. Currently this isn't an issue as
>>> those missed field belong to reserved field, just in case reserved
>>> field will be used in the future.
>>>
>>> Fixes: 1733eebb9e75b ("virtio-iommu: Implement RESV_MEM probe request")
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> nice catch.
>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> the spec is pretty confusing here though (virtio-v1.2-csd01.pdf) as it
>> presents the struct as follows:
>>
>> struct virtio_iommu_req_probe {
>> struct virtio_iommu_req_head head;
>> /* Device-readable */
>> le32 endpoint;
>> u8 reserved[64];
>>
>> /* Device-writable */
>> u8 properties[probe_size];
>> struct virtio_iommu_req_tail tail;
>> };
> Hm, which part is confusing?  Yes it's not valid C since probe_size is
> defined dynamically ('probe_size' in the device config), but I thought it
> would be nicer to show the whole request layout this way. Besides, at
> least virtio-blk and virtio-scsi have similar variable-sized arrays in
> their definitions
the fact "struct virtio_iommu_req_tail tail;" was part of the

virtio_iommu_req_probe struct

>
>> Adding Jean in the loop ...
>>
>> Thanks!
>>
>> Eric
>>
>>
>>
>>
>>> ---
>>>  v2: keep bugfix change and drop cleanup change
>>>
>>>  hw/virtio/virtio-iommu.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 7c122ab95780..195f909620ab 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -708,7 +708,8 @@ static int virtio_iommu_handle_probe(VirtIOIOMMU *s,
>>>                                       uint8_t *buf)
>>>  {
>>>      struct virtio_iommu_req_probe req;
>>> -    int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req, sizeof(req));
>>> +    int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req,
>>> +                    sizeof(req) + sizeof(struct virtio_iommu_req_tail));
> Not sure this is correct, because what we are doing here is reading the
> device-readable part of the property from the request. That part is only
> composed of fields 'head', 'endpoint' and 'reserved[64]' and its size is
> indeed sizeof(struct virtio_iommu_req_probe).
>
> The 'properties' and 'tail' fields shouldn't be read by the device here,
> they are instead written later. It is virtio_iommu_handle_command() that
> copies both of them into the request:
>
>             output_size = s->config.probe_size + sizeof(tail);
>             buf = g_malloc0(output_size);
>
>             ptail = (struct virtio_iommu_req_tail *)
>                         (buf + s->config.probe_size);
>             ptail->status = virtio_iommu_handle_probe(s, iov, iov_cnt, buf);
> 	    // and virtio_iommu_probe() fills 'properties' as needed
> 	    ...
>
> 	// then copy the lot
>         sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
>                           buf ? buf : &tail, output_size);
>
> So I think the current code is correct, as all fields are accounted for

In virtio_iommu_iov_to_req(), payload_sz is computed as

payload_sz = req_sz - sizeof(struct virtio_iommu_req_tail);

sz = iov_to_buf(iov, iov_cnt, 0, req, payload_sz);

This works for other command structs but not for probe one.

Eric


>
> Thanks,
> Jean
>
>>>  
>>>      return ret ? ret : virtio_iommu_probe(s, &req, buf);
>>>  }