On Mon, Jun 1, 2020 at 10:47 AM Mauricio Faria de Oliveira
<mfo@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
