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));
} while (!virtqueue_enable_cb(vq));
--
2.34.1
