Re: Re: [PATCH v7 04/12] virtio-blk: Add validation for block size in config space

From: Yongji Xie <xieyongji@bytedance.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Stefano Garzarella" <sgarzare@redhat.com>,
	"Parav Pandit" <parav@nvidia.com>,
	"Christoph Hellwig" <hch@infradead.org>,
	"Christian Brauner" <christian.brauner@canonical.com>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Matthew Wilcox" <willy@infradead.org>,
	viro@zeniv.linux.org.uk, "Jens Axboe" <axboe@kernel.dk>,
	bcrl@kvack.org, "Jonathan Corbet" <corbet@lwn.net>,
	"Mika Penttilä" <mika.penttila@nextfour.com>,
	joro@8bytes.org,
	virtualization <virtualization@lists.linux-foundation.org>,
	netdev@vger.kernel.org, kvm <kvm@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org, iommu@lists.linux-foundation.org,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH v7 04/12] virtio-blk: Add validation for block size in config space
Date: Thu, 20 May 2021 13:25:16 +0800	[thread overview]
Message-ID: <CACycT3veubBFCg9omxLDJJFP7B7QH8++Q+tKmb_M_hmNS45cmw@mail.gmail.com> (raw)
In-Reply-To: <20210519144206.GF32682@kadam>

On Wed, May 19, 2021 at 10:42 PM Dan Carpenter <dan.carpenter@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@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@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.

> A minimum block size of 1 seems pretty crazy.  Surely the minimum should be > higher?
>

Yes, 512 is better here.

Thanks,
Yongji