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  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:   Tue, 5 Oct 2021 23:45:31 +0800
From:   Yongji Xie <xieyongji@...edance.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        linux-block@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Kevin Wolf <kwolf@...hat.com>, Christoph Hellwig <hch@....de>,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH v5] virtio-blk: Add validation for block size in config space

On Tue, Oct 5, 2021 at 6:42 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote:
> > > An untrusted device might presents an invalid block size
> > > in configuration space. This tries to add validation for it
> > > in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
> > > feature bit if the value is out of the supported range.
> > >
> > > And we also double check the value in virtblk_probe() in
> > > case that it's changed after the validation.
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@...edance.com>
> >
> > So I had to revert this due basically bugs in QEMU.
> >
> > My suggestion at this point is to try and update
> > blk_queue_logical_block_size to BUG_ON when the size
> > is out of a reasonable range.
> >
> > This has the advantage of fixing more hardware, not just virtio.
> >
>
> Stefan also pointed out this duplicates the logic from
>
>         if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize))
>                 return -EINVAL;
>
>
> and a bunch of other places.
>
>
> Would it be acceptable for blk layer to validate the input
> instead of having each driver do it's own thing?
> Maybe inside blk_queue_logical_block_size?
>

Now the nbd and loop driver will validate the blksize and return error
if it's invalid. But the nvme and null_blk driver just corrects the
blksize to a reasonable range without returning any error. I'm not
sure which way the block layer should follow. Or just simplify the
logic in nbd and loop driver?

Thanks,
Yongji

Powered by blists - more mailing lists