Re: [PATCH 2/2] virtio-pci: Add admin command length check for device writable portions

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Apr 21, 2025 at 06:00:27PM +0300, Israel Rukshin wrote:
> On 4/21/2025 5:12 PM, Michael S. Tsirkin wrote:
> > On Mon, Apr 21, 2025 at 03:36:21PM +0300, Israel Rukshin wrote:
> > > On 4/21/2025 2:29 PM, Michael S. Tsirkin wrote:
> > > > 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 <israelr@nvidia.com>
> > > > > > > > > Reviewed-by: Max Gurtovoy <mgurtovoy@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?
> > > > 
> > > Sorry for the confusion.
> > > 
> > > This change is essentially PATCH 1/2 from this series with the addition of
> > > handling the case when len is smaller than status_size to avoid getting a
> > > negative value.
> > > According to your comments, I will remove PATCH 2/2 and post only PATCH 1/2
> > > with this change.
> > > You are correct, this is a userspace-visible change, and the size currently
> > > reported in virtio_pci_admin_dev_parts_get() is incorrect.
> > 
> > Sounds good. My only question is, how bad is the current condition?
> The migration fails if the state size is exactly 4KB, because the allocated
> buffer (4KB) is smaller than the amount the user is trying to read (4KB + 8
> bytes).
> For other state sizes, there are an extra 8 bytes of data at the end of the
> state.
> According to the specification, the device at the destination must fail the
> migration because this is not a valid state.
> > is there a chance userspace will try to work around it?
> I am not aware of.

My concern is userspace might try to work around this bug..
Do you want to add a module attribute to tell userspace it's fixed?


> > 
> > 
> > > > > > > > >      	} while (!virtqueue_enable_cb(vq));
> > > > > > > > > -- 
> > > > > > > > > 2.34.1