Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Yongji Xie]

On Mon, Oct 4, 2021 at 11:39 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >
> > 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.
> >
> >
> >
> > > ---
> > >  drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
> > >  1 file changed, 33 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index 4b49df2dfd23..afb37aac09e8 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
> > >  static unsigned int virtblk_queue_depth;
> > >  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> > >
> > > +static int virtblk_validate(struct virtio_device *vdev)
> > > +{
> > > +   u32 blk_size;
> > > +
> > > +   if (!vdev->config->get) {
> > > +           dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > > +                   __func__);
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > > +           return 0;
> > > +
> > > +   blk_size = virtio_cread32(vdev,
> > > +                   offsetof(struct virtio_blk_config, blk_size));
> > > +
> > > +   if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > > +           __virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >  static int virtblk_probe(struct virtio_device *vdev)
> > >  {
> > >     struct virtio_blk *vblk;
>
> I started wondering about this. So let's assume is
> PAGE_SIZE < blk_size (after all it's up to guest at many platforms).
>
> Will using the device even work given blk size is less than what
> is can support?
>
> And what exactly happens today if blk_size is out of this range?
>

Now the block layer can't support the block size larger than the page
size. Otherwise, it would cause a random crash, e.g. in
block_read_full_page().

Thanks,
Yongji