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

[Jean-Philippe Brucker]

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

> 
> 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

Thanks,
Jean

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