On Mon, Apr 21, 2025 at 02:05:27PM +0300, Israel Rukshin wrote:
> On 4/20/2025 4:07 PM, Michael S. Tsirkin wrote:
> > 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.
>
> What do you think on the bellow change for v2?
>
> @@ -56,7 +57,13 @@ 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;
> + /* If the length written is less than the status
> size,
> + * the remaining status bytes stay zero-initialized.
> + */
> + if (len < status_size)
> + cmd->result_sg_size = 0;
> + else
> + cmd->result_sg_size = len - status_size;
> complete(&cmd->completion);
> }
> } while (!virtqueue_enable_cb(vq));
Can you please explain what exactly is this change doing?
The changelog claims all that is done is adding a check,
but that's not true: previously we reported the whole length. Now you want to
report
length less status_size. This is a userspace-visible change, is it not?
> > > >
> > > >
> > > > > } while (!virtqueue_enable_cb(vq));
> > > > > --
> > > > > 2.34.1
