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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 May 2021 15:08:30 +0800
From:   Yongji Xie <xieyongji@...edance.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        Jason Wang <jasowang@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Parav Pandit <parav@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Christian Brauner <christian.brauner@...onical.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Matthew Wilcox <willy@...radead.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jens Axboe <axboe@...nel.dk>, bcrl@...ck.org,
        Jonathan Corbet <corbet@....net>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        joro@...tes.org,
        virtualization <virtualization@...ts.linux-foundation.org>,
        netdev@...r.kernel.org, kvm <kvm@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, iommu@...ts.linux-foundation.org,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Re: Re: [PATCH v7 04/12] virtio-blk: Add validation for block
 size in config space

On Thu, May 20, 2021 at 1:43 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> 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@...cle.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@...edance.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@...edance.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.

But it seems better if we can avoid this kind of panic. Because this
might also be triggered by a buggy VDUSE daemon.

> Let's focus on eliminating data corruption for starters.

OK, now the incorrect used length might cause data corruption in
virtio-net and virtio-console drivers as I mentioned in another mail.
I will send a fix ASAP.

Thanks,
Yongji