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

[Max Gurtovoy <mgurtovoy@nvidia.com>]

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)


>