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

[Jean-Philippe Brucker]

On Wed, Jun 22, 2022 at 02:22:18PM +0200, Eric Auger wrote:
> >> 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

Right, it would have been better to use a different name than
virtio_iommu_req_probe in virtio_iommu.h, to make the pitfall clear.

The larger problem is using C structs across the virtio spec instead of an
abstract format. Someone implementing the device in another language would
already not encounter this problem since they would read the spec as an
abstract format. For documentation purposes I do prefer displaying the
whole struct like this rather than working around limitations of C, which
may be more confusing.


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

Aah right sorry. The resulting code may be less confusing if we moved
"- sizeof(struct virtio_iommu_req_tail)" to virtio_iommu_handle_req()

Thanks,
Jean