[PATCH v2] nvme: check for valid data from from nvme_identify_ns() before using it

[Ewan Milne]

Sure, the kernel should not crash regardless of what any device reports,
especially an external one.

The _identify_ns() divide by zero happened because the NVMe spec
says a page of zeroes can be returned if a namespace isn't configured.
I'm not really sure why the spec didn't require a nonzero status code.

-Ewan

On Tue, Nov 28, 2023 at 12:32 PM Keith Busch <kbusch at kernel.org> wrote:
>
> On Tue, Nov 28, 2023 at 11:10:29AM -0500, Ewan Milne wrote:
> > > Interestingly enough, I think this is the same as what was recently
> > > reported here:
> > >
> > >   https://bugzilla.kernel.org/show_bug.cgi?id=218186
> > >
> >
> > Yes, from the stack trace and BZ comments it looks like it.
>
> Slightly related, there's nothing stopping a device from reporting a
> bogus LBA shift value. We're only checking for "too big", but neglect to
> check for too small, and may similiarly shift off to a division by 0. We
> might need a check like this too:
>
> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a55c2a774b9c4..8bf78f876f7ac 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1903,7 +1903,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>          * The block layer can't support LBA sizes larger than the page size
>          * yet, so catch this early and don't allow block I/O.
>          */
> -       if (ns->lba_shift > PAGE_SHIFT) {
> +       if (ns->lba_shift > PAGE_SHIFT || ns->lba_shift < SECTOR_SHIFT) {
>                 capacity = 0;
>                 bs = (1 << 9);
>         }
> --
>