Re: [PATCH] block: check for page size in queue_logical_block_size()

From: Mauricio Faria de Oliveira <mfo@canonical.com>
To: linux-block@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	Mikulas Patocka <mpatocka@redhat.com>,
	Ming Lei <ming.lei@redhat.com>
Subject: Re: [PATCH] block: check for page size in queue_logical_block_size()
Date: Wed, 3 Jun 2020 08:33:14 -0300	[thread overview]
Message-ID: <CAO9xwp07aQ_hDCS-MKwqy8h0w3ZyHwbeku2w6OussWO6wxKVhw@mail.gmail.com> (raw)
In-Reply-To: <CAO9xwp0mibE5_cpq4qaGtJBMBbouUf+jmJEQv7jF5DiL71CCjg@mail.gmail.com>

On Mon, Jun 1, 2020 at 10:47 AM Mauricio Faria de Oliveira
<mfo@canonical.com> wrote:
>
> Hi Ming,
>
> (sorry, re-sending in plain text; previous reply had HTML by mistake,
> and bounced in linux-block.)
>
> On Mon, Jun 1, 2020 at 4:34 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Sun, May 31, 2020 at 09:55:20PM -0300, Mauricio Faria de Oliveira wrote:
> > > It's possible for a block driver to set logical block size to
> > > a value greater than page size incorrectly; e.g. bcache takes
> > > the value from the superblock, set by the user w/ make-bcache.
> > >
> > > This causes a BUG/NULL pointer dereference in the path:
> > >
> > >   __blkdev_get()
> > >   -> set_init_blocksize() // set i_blkbits based on ...
> > >      -> bdev_logical_block_size()
> > >         -> queue_logical_block_size() // ... this value
> > >   -> bdev_disk_changed()
> > >      ...
> > >      -> blkdev_readpage()
> > >         -> block_read_full_page()
> > >            -> create_page_buffers() // size = 1 << i_blkbits
> > >               -> create_empty_buffers() // give size/take pointer
> > >                  -> alloc_page_buffers() // return NULL
> > >                  .. BUG!
> > >
> > > Because alloc_page_buffers() is called with size > PAGE_SIZE,
> > > thus it initializes head = NULL, skips the loop, return head;
> > > then create_empty_buffers() gets (and uses) the NULL pointer.
> > >
> > > This has been around longer than commit ad6bf88a6c19 ("block:
> > > fix an integer overflow in logical block size"); however, it
> > > increased the range of values that can trigger the issue.
> > >
> > > Previously only 8k/16k/32k (on x86/4k page size) would do it,
> > > as greater values overflow unsigned short to zero, and queue_
> > > logical_block_size() would then use the default of 512.
> > >
> > > Now the range with unsigned int is much larger, and one user
> > > with an (incorrect) 512k value, which happened to be zero'ed
> > > previously and work fine, hits the issue -- the zero is gone,
> > > and queue_logical_block_size() does return 512k (> PAGE_SIZE)
> >
> > There is only very limited such potential users(loop, virtio-blk,
> > xen-blkfront), so could you fix the user instead of working around
> > queue_logical_block_size()?
> >
>
> Thanks for reviewing.
>
> I can take a look at that, sure, but think the current approach may
> still be useful? as it prevents the current, and future potential
> users too.
>

Please disregard this patch.

Giving this more thought, it's not a good idea to "prevent" any issues
here -- that would actually mask them.
It's probably better to let current issues break, to identify and fix
them (e.g., this), and especially future issues, to hit/fix before
landing.

Thanks,

> Cheers,
>
> > thanks,
> > Ming
> >
>
>
> --
> Mauricio Faria de Oliveira

-- 
Mauricio Faria de Oliveira