Re: [PATCH] nvme: core: reject invalid LBA data size from Identify Namespace

[Chao S]

Hi Maurizio,

Thanks for the nit on v1.

> If I'm reading the NVMe spec correctly, ds == 0 has a special
> meaning: 'LBA format is not currently available.'
> maybe we should use a different dev_warn() for ds == 0 ?

I added the separate warning in v2, but Keith pointed out on the
v2 thread that the "LBA format not available" wording in the spec
refers to broadcast identification.  For an attached namespace,
any unusable flbas index value (including 0) is invalid, so v3
merges both cases into a single check.

Apologies for the back-and-forth.

Also: v2 bounced to your old redhat.com address — using
mlombard@xxxxxxxxxx from here on.

Thanks,
Chao

On Mon, Apr 20, 2026 at 8:22 AM Maurizio Lombardi <mlombard@xxxxxxxxxx> wrote:
>
> On Sat Apr 18, 2026 at 6:28 AM CEST, Chao Shi wrote:
> > nvme_update_ns_info_block() trusts id->lbaf[lbaf].ds from the
> > controller and assigns it directly to ns->head->lba_shift without
> > bounds checking.  nvme_lba_to_sect() then does:
> >
> >     return lba << (head->lba_shift - SECTOR_SHIFT);
> >
> > When called with lba = le64_to_cpu(id->nsze) to compute the device
> > capacity, an attacker-controlled controller can choose ds < 9 or a
> > combination of (ds, nsze) that makes the left shift overflow
> > sector_t.  The former is a C undefined behaviour that UBSAN reports
> > as a BUG; the latter silently yields a bogus capacity that the
> > block layer then trusts for bounds checking.
> >
> > Validate ds against SECTOR_SHIFT and use check_shl_overflow() to
> > compute capacity so that any (ds, nsze) combination that would
> > overflow sector_t is rejected.  The namespace is skipped with -EIO
> > instead of crashing the kernel.  This is reachable by a malicious
> > NVMe device, a buggy firmware, or an attacker-controlled NVMe-oF
> > target.
> >
> > Stack trace (UBSAN, ds < 9 variant):
> >
> >   RIP: nvme_lba_to_sect drivers/nvme/host/nvme.h:699 [inline]
> >   RIP: nvme_update_ns_info_block.cold+0x5/0x7
> >   Call Trace:
> >    nvme_update_ns_info+0x175/0xd90 drivers/nvme/host/core.c:2467
> >    nvme_validate_ns drivers/nvme/host/core.c:4299 [inline]
> >    nvme_scan_ns drivers/nvme/host/core.c:4350
> >    nvme_scan_ns_async+0xa5/0xe0 drivers/nvme/host/core.c:4383
> >    async_run_entry_fn
> >    process_one_work
> >    worker_thread
> >    kthread
> >
> > Found by Syzkaller.
> >
> > Fixes: 9419e71b8d67 ("nvme: move ns id info to struct nvme_ns_head")
> > Acked-by: Sungwoo Kim <iam@xxxxxxxxxxxx>
> > Acked-by: Dave Tian <daveti@xxxxxxxxxx>
> > Acked-by: Weidong Zhu <weizhu@xxxxxxx>
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
> > ---
> >  drivers/nvme/host/core.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 1e33af94c24..9b3bf3e4075 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -2407,9 +2407,19 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
> >       lim = queue_limits_start_update(ns->disk->queue);
> >
> >       memflags = blk_mq_freeze_queue(ns->disk->queue);
> > +     if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> > +         check_shl_overflow(le64_to_cpu(id->nsze),
> > +                            id->lbaf[lbaf].ds - SECTOR_SHIFT,
> > +                            &capacity)) {
> > +             dev_warn_once(ns->ctrl->device,
> > +                     "invalid LBA data size %u, skipping namespace\n",
> > +                     id->lbaf[lbaf].ds);
>
> Just a nit:
>
> If I'm reading the NVMe spec correctly, ds == 0 has a special meaning:
> 'LBA format is not currently available.'
> maybe we should use a different dev_warn() for ds == 0 ?
>
> Maurizio
>