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@xxxxxxxxxx
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)
