Michael S. Tsirkin
2021-May-20 05:43 UTC
[PATCH v7 04/12] virtio-blk: Add validation for block size in config space
On Thu, May 20, 2021 at 01:25:16PM +0800, Yongji Xie wrote:> On Wed, May 19, 2021 at 10:42 PM Dan Carpenter <dan.carpenter at oracle.com> wrote: > > > > On Wed, May 19, 2021 at 09:39:20PM +0800, Yongji Xie wrote: > > > On Mon, May 17, 2021 at 5:56 PM Xie Yongji <xieyongji at bytedance.com> wrote: > > > > > > > > This ensures that we will not use an invalid block size > > > > in config space (might come from an untrusted device). > > > > I looked at if I should add this as an untrusted function so that Smatch > > could find these sorts of bugs but this is reading data from the host so > > there has to be some level of trust... > > > > It would be great if Smatch could detect this case if possible. The > data might be trusted in traditional VM cases. But now the data can be > read from a userspace daemon when VDUSE is enabled. > > > I should add some more untrusted data kvm functions to Smatch. Right > > now I only have kvm_register_read() and I've added kvm_read_guest_virt() > > just now. > > > > > > > > > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > > > > --- > > > > drivers/block/virtio_blk.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > > > index ebb4d3fe803f..c848aa36d49b 100644 > > > > --- a/drivers/block/virtio_blk.c > > > > +++ b/drivers/block/virtio_blk.c > > > > @@ -826,7 +826,7 @@ static int virtblk_probe(struct virtio_device *vdev) > > > > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, > > > > struct virtio_blk_config, blk_size, > > > > &blk_size); > > > > - if (!err) > > > > + if (!err && blk_size > 0 && blk_size <= max_size) > > > > > > The check here is incorrect. I will use PAGE_SIZE as the maximum > > > boundary in the new version. > > > > What does this bug look like to the user? > > The kernel will panic if the block size is larger than PAGE_SIZE.Kernel panic at this point is par for the course IMHO. Let's focus on eliminating data corruption for starters.> > A minimum block size of 1 seems pretty crazy. Surely the minimum should be > higher? > > > > Yes, 512 is better here. > > Thanks, > Yongji