On Sun, Apr 20, 2025 at 03:31:25PM +0300, Max Gurtovoy wrote:
>
> On 20/04/2025 14:33, Michael S. Tsirkin wrote:
> > On Sun, Apr 20, 2025 at 02:19:05PM +0300, Israel Rukshin wrote:
> > > Add a safety check to ensure that the length of data written by the
> > > device is at least as large as the status length. If this condition is
> > > not met, it indicates a potential error in the device's response.
> > >
> > > This change aligns with the virtio specification, which states:
> > > "The driver MUST NOT make assumptions about data in device-writable
> > > buffers beyond the first len bytes, and SHOULD ignore this data."
> > >
> > > By failing the admin command when len is insufficient, we ensure
> > > that the driver does not process potentially invalid or incomplete
> > > status from the device.
> > >
> > > Signed-off-by: Israel Rukshin <isra...@nvidia.com>
> > > Reviewed-by: Max Gurtovoy <mgurto...@nvidia.com>
> > > ---
> > > drivers/virtio/virtio_pci_modern.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c
> > > b/drivers/virtio/virtio_pci_modern.c
> > > index 7209390a5cbf..0374e86aece3 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -48,6 +48,7 @@ void vp_modern_avq_done(struct virtqueue *vq)
> > > {
> > > struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
> > > + unsigned int status_size = sizeof(struct virtio_admin_cmd_status);
> > > struct virtio_admin_cmd *cmd;
> > > unsigned long flags;
> > > unsigned int len;
> > > @@ -56,8 +57,10 @@ void vp_modern_avq_done(struct virtqueue *vq)
> > > do {
> > > virtqueue_disable_cb(vq);
> > > while ((cmd = virtqueue_get_buf(vq, &len))) {
> > > - cmd->result_sg_size =
> > > - len - sizeof(struct virtio_admin_cmd_status);
> > > + if (len < status_size)
> > > + cmd->ret = -EIO;
> > > + else
> > > + cmd->result_sg_size = len - status_size;
> > > complete(&cmd->completion);
> > > }
> >
> >
> > No this is out of spec:
> > Similarly, the driver compares the used buffer length
> > of the buffer to what it expects and then silently
> > truncates the structure to the used buffer length.
> > The driver silently ignores any data falling outside
> > the used buffer length reported by the device. Any missing
> > fields are interpreted as set to zero.
> >
> > so if status is 0, device can just write a shorter buffer and
> > driver must assume 0, which is success, not failure.
>
> maybe we can make a sanity check that the len is at least 4B (status and
> status_qualifier) which is basic size to make sure device is acting as
> expected ?
>
The spec says nothing about this. Just zero-initialize the buffer
beyond len and be done with it.
>
> >
> >
> >
> > > } while (!virtqueue_enable_cb(vq));
> > > --
> > > 2.34.1
