| Message ID | 20250224233106.8519-1-mgurtovoy@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtio: Add length checks for device writable portions | expand |
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
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.
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) >