[PATCH v1 0/2] virtio: Add length checks for device writable portions

[PATCH v1 0/2] virtio: Add length checks for device writable portions

[Max Gurtovoy]

Hi,

This patch series introduces safety checks in virtio-blk and virtio-fs
drivers to ensure proper handling of device-writable buffer lengths as
specified by the virtio specification.

The virtio specification states:
"The driver MUST NOT make assumptions about data in device-writable
buffers beyond the first len bytes, and SHOULD ignore this data."

To align with this requirement, we introduce checks in both drivers to
verify that the length of data written by the device is at least as
large as the expected/needed payload.

If this condition is not met, we set an I/O error status to prevent
processing of potentially invalid or incomplete data.

These changes improve the robustness of the drivers and ensure better
compliance with the virtio specification.

Max Gurtovoy (2):
  virtio_blk: add length check for device writable portion
  virtio_fs: add length check for device writable portion

 drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
 fs/fuse/virtio_fs.c        |  9 +++++++++
 2 files changed, 29 insertions(+)

-- 
2.18.1

[PATCH 1/2] virtio_blk: add length check for device writable portion

[Max Gurtovoy]

Add a safety check to ensure that the length of data written by the
device is at least as large the expected 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 setting an error status when len is insufficient, we ensure that the
driver does not process potentially invalid or incomplete data from the
device.

Reviewed-by: Aurelien Aptel <aaptel@nvidia.com>
Signed-off-by: Lokesh Arora <larora@nvidia.com>
Signed-off-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 6a61ec35f426..58407cfee3ee 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -331,6 +331,20 @@ static inline u8 virtblk_vbr_status(struct virtblk_req *vbr)
 	return *((u8 *)&vbr->in_hdr + vbr->in_hdr_len - 1);
 }
 
+static inline void virtblk_vbr_set_err_status_upon_len_err(struct virtblk_req *vbr,
+		struct request *req, unsigned int len)
+{
+	unsigned int expected_len = vbr->in_hdr_len;
+
+	if (rq_dma_dir(req) == DMA_FROM_DEVICE)
+		expected_len += blk_rq_payload_bytes(req);
+
+	if (unlikely(len < expected_len)) {
+		u8 *status_ptr = (u8 *)&vbr->in_hdr + vbr->in_hdr_len - 1;
+		*status_ptr = VIRTIO_BLK_S_IOERR;
+	}
+}
+
 static inline void virtblk_request_done(struct request *req)
 {
 	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
@@ -362,6 +376,9 @@ static void virtblk_done(struct virtqueue *vq)
 		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
 			struct request *req = blk_mq_rq_from_pdu(vbr);
 
+			/* Check device writable portion length, and fail upon error */
+			virtblk_vbr_set_err_status_upon_len_err(vbr, req, len);
+
 			if (likely(!blk_should_fake_timeout(req->q)))
 				blk_mq_complete_request(req);
 			req_done = true;
@@ -1208,6 +1225,9 @@ static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 	while ((vbr = virtqueue_get_buf(vq->vq, &len)) != NULL) {
 		struct request *req = blk_mq_rq_from_pdu(vbr);
 
+		/* Check device writable portion length, and fail upon error */
+		virtblk_vbr_set_err_status_upon_len_err(vbr, req, len);
+
 		found++;
 		if (!blk_mq_complete_request_remote(req) &&
 		    !blk_mq_add_to_batch(req, iob, virtblk_vbr_status(vbr),
-- 
2.18.1

[PATCH 2/2] virtio_fs: add length check for device writable portion

[Max Gurtovoy]

Add a safety check to ensure that the length of data written by the
device is at least as large the expected 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 setting an error status when len is insufficient, we ensure that
the driver does not process potentially invalid or incomplete data from
the device.

Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
Signed-off-by: Lokesh Arora <larora@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 fs/fuse/virtio_fs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 82afe78ec542..658e51e41292 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -827,6 +827,15 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 		virtqueue_disable_cb(vq);
 
 		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
+
+			/*
+			 * Check device writable portion length, and fail upon
+			 * error.
+			 */
+			if (unlikely(len < sizeof(req->out.h) ||
+			    len < req->out.h.len))
+				req->out.h.error = -EIO;
+
 			spin_lock(&fpq->lock);
 			list_move_tail(&req->list, &reqs);
 			spin_unlock(&fpq->lock);
-- 
2.18.1

Re: [PATCH v1 0/2] virtio: Add length checks for device writable portions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Tue, Feb 25, 2025 at 01:31:04AM +0200, Max Gurtovoy wrote:
> Hi,
> 
> This patch series introduces safety checks in virtio-blk and virtio-fs
> drivers to ensure proper handling of device-writable buffer lengths as
> specified by the virtio specification.
> 
> The virtio specification states:
> "The driver MUST NOT make assumptions about data in device-writable
> buffers beyond the first len bytes, and SHOULD ignore this data."
> 
> To align with this requirement, we introduce checks in both drivers to
> verify that the length of data written by the device is at least as
> large as the expected/needed payload.
> 
> If this condition is not met, we set an I/O error status to prevent
> processing of potentially invalid or incomplete data.
> 
> These changes improve the robustness of the drivers and ensure better
> compliance with the virtio specification.
> 
> Max Gurtovoy (2):
>   virtio_blk: add length check for device writable portion
>   virtio_fs: add length check for device writable portion
> 
>  drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
>  fs/fuse/virtio_fs.c        |  9 +++++++++
>  2 files changed, 29 insertions(+)
> 
> -- 
> 2.18.1
> 

There are 3 cases:
1. The device reports len correctly.
2. The device reports len incorrectly, but the in buffers contain valid
   data.
3. The device reports len incorrectly and the in buffers contain invalid
   data.

Case 1 does not change behavior.

Case 3 never worked in the first place. This patch might produce an
error now where garbage was returned in the past.

It's case 2 that I'm worried about: users won't be happy if the driver
stops working with a device that previously worked.

Should we really risk breakage for little benefit?

I remember there were cases of invalid len values reported by devices in
the past. Michael might have thoughts about this.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/2] virtio: Add length checks for device writable portions

[Michael S. Tsirkin]

On Thu, Feb 27, 2025 at 04:17:47PM +0800, Stefan Hajnoczi wrote:
> On Tue, Feb 25, 2025 at 01:31:04AM +0200, Max Gurtovoy wrote:
> > Hi,
> > 
> > This patch series introduces safety checks in virtio-blk and virtio-fs
> > drivers to ensure proper handling of device-writable buffer lengths as
> > specified by the virtio specification.
> > 
> > The virtio specification states:
> > "The driver MUST NOT make assumptions about data in device-writable
> > buffers beyond the first len bytes, and SHOULD ignore this data."
> > 
> > To align with this requirement, we introduce checks in both drivers to
> > verify that the length of data written by the device is at least as
> > large as the expected/needed payload.
> > 
> > If this condition is not met, we set an I/O error status to prevent
> > processing of potentially invalid or incomplete data.
> > 
> > These changes improve the robustness of the drivers and ensure better
> > compliance with the virtio specification.
> > 
> > Max Gurtovoy (2):
> >   virtio_blk: add length check for device writable portion
> >   virtio_fs: add length check for device writable portion
> > 
> >  drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
> >  fs/fuse/virtio_fs.c        |  9 +++++++++
> >  2 files changed, 29 insertions(+)
> > 
> > -- 
> > 2.18.1
> > 
> 
> There are 3 cases:
> 1. The device reports len correctly.
> 2. The device reports len incorrectly, but the in buffers contain valid
>    data.
> 3. The device reports len incorrectly and the in buffers contain invalid
>    data.
> 
> Case 1 does not change behavior.
> 
> Case 3 never worked in the first place. This patch might produce an
> error now where garbage was returned in the past.
> 
> It's case 2 that I'm worried about: users won't be happy if the driver
> stops working with a device that previously worked.
> 
> Should we really risk breakage for little benefit?
> 
> I remember there were cases of invalid len values reported by devices in
> the past. Michael might have thoughts about this.
> 
> Stefan


Indeed, there were. This is where Jason's efforts to validate
length stalled.

See message id 20230526063041.18359-1-jasowang@redhat.com

I am not sure I get the motivation for this patch. And yes, seems to
risky especially for blk. If it's to help device validation, I suggest a
Kconfig option.


-- 
MST

Re: [PATCH v1 0/2] virtio: Add length checks for device writable portions

[Max Gurtovoy]

On 27/02/2025 10:53, Michael S. Tsirkin wrote:
> On Thu, Feb 27, 2025 at 04:17:47PM +0800, Stefan Hajnoczi wrote:
>> On Tue, Feb 25, 2025 at 01:31:04AM +0200, Max Gurtovoy wrote:
>>> Hi,
>>>
>>> This patch series introduces safety checks in virtio-blk and virtio-fs
>>> drivers to ensure proper handling of device-writable buffer lengths as
>>> specified by the virtio specification.
>>>
>>> The virtio specification states:
>>> "The driver MUST NOT make assumptions about data in device-writable
>>> buffers beyond the first len bytes, and SHOULD ignore this data."
>>>
>>> To align with this requirement, we introduce checks in both drivers to
>>> verify that the length of data written by the device is at least as
>>> large as the expected/needed payload.
>>>
>>> If this condition is not met, we set an I/O error status to prevent
>>> processing of potentially invalid or incomplete data.
>>>
>>> These changes improve the robustness of the drivers and ensure better
>>> compliance with the virtio specification.
>>>
>>> Max Gurtovoy (2):
>>>    virtio_blk: add length check for device writable portion
>>>    virtio_fs: add length check for device writable portion
>>>
>>>   drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
>>>   fs/fuse/virtio_fs.c        |  9 +++++++++
>>>   2 files changed, 29 insertions(+)
>>>
>>> -- 
>>> 2.18.1
>>>
>> There are 3 cases:
>> 1. The device reports len correctly.
>> 2. The device reports len incorrectly, but the in buffers contain valid
>>     data.
>> 3. The device reports len incorrectly and the in buffers contain invalid
>>     data.
>>
>> Case 1 does not change behavior.
>>
>> Case 3 never worked in the first place. This patch might produce an
>> error now where garbage was returned in the past.
>>
>> It's case 2 that I'm worried about: users won't be happy if the driver
>> stops working with a device that previously worked.
>>
>> Should we really risk breakage for little benefit?
>>
>> I remember there were cases of invalid len values reported by devices in
>> the past. Michael might have thoughts about this.
>>
>> Stefan
>
> Indeed, there were. This is where Jason's efforts to validate
> length stalled.
>
> See message id 20230526063041.18359-1-jasowang@redhat.com
>
> I am not sure I get the motivation for this patch. And yes, seems to
> risky especially for blk. If it's to help device validation, I suggest a
> Kconfig option.

The primary motivation for this patch is to improve spec compliance and 
enhance driver robustness.
You're right that there are different cases to consider:

     1. For correctly behaving devices (Case 1) and fully incorrectly 
behaving devices (Case 3), there's no change in behavior (Case 3 never 
worked anyway).
     2. For devices reporting incorrect lengths but with valid data 
(Case 2), I understand the concern about breaking existing setups.

To address the concerns about Case 2 while still moving towards better 
spec compliance,we can make them configurable, as Michael suggested.
Some configuration options:

     1. Via a Kconfig
     2. Via module param
     3. Via adding quirks for known non-compliant devices (identified by 
subsystem dev/vendor ids) that are otherwise functional.
     4. Via virtio_has_feature(vdev, VIRTIO_F_VERSION_1)


>

Re: [PATCH v1 0/2] virtio: Add length checks for device writable portions

[Michael S. Tsirkin]

On Thu, Feb 27, 2025 at 04:17:47PM +0800, Stefan Hajnoczi wrote:
> On Tue, Feb 25, 2025 at 01:31:04AM +0200, Max Gurtovoy wrote:
> > Hi,
> > 
> > This patch series introduces safety checks in virtio-blk and virtio-fs
> > drivers to ensure proper handling of device-writable buffer lengths as
> > specified by the virtio specification.
> > 
> > The virtio specification states:
> > "The driver MUST NOT make assumptions about data in device-writable
> > buffers beyond the first len bytes, and SHOULD ignore this data."
> > 
> > To align with this requirement, we introduce checks in both drivers to
> > verify that the length of data written by the device is at least as
> > large as the expected/needed payload.
> > 
> > If this condition is not met, we set an I/O error status to prevent
> > processing of potentially invalid or incomplete data.
> > 
> > These changes improve the robustness of the drivers and ensure better
> > compliance with the virtio specification.
> > 
> > Max Gurtovoy (2):
> >   virtio_blk: add length check for device writable portion
> >   virtio_fs: add length check for device writable portion
> > 
> >  drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
> >  fs/fuse/virtio_fs.c        |  9 +++++++++
> >  2 files changed, 29 insertions(+)
> > 
> > -- 
> > 2.18.1
> > 
> 
> There are 3 cases:
> 1. The device reports len correctly.
> 2. The device reports len incorrectly, but the in buffers contain valid
>    data.
> 3. The device reports len incorrectly and the in buffers contain invalid
>    data.
> 
> Case 1 does not change behavior.
> 
> Case 3 never worked in the first place. This patch might produce an
> error now where garbage was returned in the past.
> 
> It's case 2 that I'm worried about: users won't be happy if the driver
> stops working with a device that previously worked.

Interestingly, when virtio core unmaps buffers, it always syncronizes
the whole range.
virtio net jumps through hoops to only sync a part.

So Max, my suggestion is to maybe try a combination of dma sync +
unmap, and then this becomes an optimization.

It might be worth it, for when using swiotlb - but the gain will
have to be measured.





> Should we really risk breakage for little benefit?
> 
> I remember there were cases of invalid len values reported by devices in
> the past. Michael might have thoughts about this.
> 
> Stefan

Re: [PATCH 1/2] virtio_blk: add length check for device writable portion

[Michael S. Tsirkin]

On Tue, Feb 25, 2025 at 01:31:05AM +0200, Max Gurtovoy wrote:
> Add a safety check to ensure that the length of data written by the
> device is at least as large the expected 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 setting an error status when len is insufficient, we ensure that the
> driver does not process potentially invalid or incomplete data from the
> device.
> 
> Reviewed-by: Aurelien Aptel <aaptel@nvidia.com>
> Signed-off-by: Lokesh Arora <larora@nvidia.com>
> Signed-off-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---



my question is, is the device out of spec, too?



>  drivers/block/virtio_blk.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 6a61ec35f426..58407cfee3ee 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -331,6 +331,20 @@ static inline u8 virtblk_vbr_status(struct virtblk_req *vbr)
>  	return *((u8 *)&vbr->in_hdr + vbr->in_hdr_len - 1);
>  }
>  
> +static inline void virtblk_vbr_set_err_status_upon_len_err(struct virtblk_req *vbr,
> +		struct request *req, unsigned int len)
> +{
> +	unsigned int expected_len = vbr->in_hdr_len;
> +
> +	if (rq_dma_dir(req) == DMA_FROM_DEVICE)
> +		expected_len += blk_rq_payload_bytes(req);
> +
> +	if (unlikely(len < expected_len)) {
> +		u8 *status_ptr = (u8 *)&vbr->in_hdr + vbr->in_hdr_len - 1;
> +		*status_ptr = VIRTIO_BLK_S_IOERR;
> +	}
> +}
> +
>  static inline void virtblk_request_done(struct request *req)
>  {
>  	struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
> @@ -362,6 +376,9 @@ static void virtblk_done(struct virtqueue *vq)
>  		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
>  			struct request *req = blk_mq_rq_from_pdu(vbr);
>  
> +			/* Check device writable portion length, and fail upon error */
> +			virtblk_vbr_set_err_status_upon_len_err(vbr, req, len);
> +
>  			if (likely(!blk_should_fake_timeout(req->q)))
>  				blk_mq_complete_request(req);
>  			req_done = true;
> @@ -1208,6 +1225,9 @@ static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
>  	while ((vbr = virtqueue_get_buf(vq->vq, &len)) != NULL) {
>  		struct request *req = blk_mq_rq_from_pdu(vbr);
>  
> +		/* Check device writable portion length, and fail upon error */
> +		virtblk_vbr_set_err_status_upon_len_err(vbr, req, len);
> +
>  		found++;
>  		if (!blk_mq_complete_request_remote(req) &&
>  		    !blk_mq_add_to_batch(req, iob, virtblk_vbr_status(vbr),
> -- 
> 2.18.1