Re: [PATCH v5] virtio-blk: Add validation for block size in config space
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2021-08-23 16:03:08
Also in:
lkml, virtualization
On Mon, Aug 23, 2021 at 08:40:30PM +0800, Yongji Xie wrote:
On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin [off-list ref] wrote:quoted
On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote:quoted
It helpful if there is a justification for this. In this case, no such HW device exist and the only device that can cause this trouble today is user space VDUSE device that must be validated by the emulation VDUSE kernel driver. Otherwise, will can create 1000 commit like this in the virtio level (for example for each feature for each virtio device).Yea, it's a lot of work but I don't think it's avoidable.quoted
quoted
quoted
quoted
quoted
quoted
And regardless of userspace device, we still need to fix it for other cases.which cases ? Do you know that there is a buggy HW we need to workaround ?No, there isn't now. But this could be a potential attack surface if the host doesn't trust the device.If the host doesn't trust a device, why it continues using it ?IIUC this is the case for the encrypted VMs.what do you mean encrypted VM ? And how this small patch causes a VM to be 100% encryption supported ?quoted
quoted
Do you suggest we do these workarounds in all device drivers in the kernel ?Isn't it the driver's job to validate some unreasonable configuration?The check should be in different layer. Virtio blk driver should not cover on some strange VDUSE stuff.Yes I'm not convinced VDUSE is a valid use-case. I think that for security and robustness it should validate data it gets from userspace right there after reading it. But I think this is useful for the virtio hardening thing. https://lwn.net/Articles/865216/ Yongji - I think the commit log should be much more explicit that this is hardening. Otherwise people get confused and think this needs a CVE or a backport for security.OK, do I need to send a v6? This patch seems to be already merged into Linus's tree. Thanks, Yongji
No, it's a comment for the future - I assume you will keep adding this kind of validation in other places. -- MST