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

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

[Chao Shi]

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
-ENODEV instead of crashing the kernel.  This is reachable by a
malicious NVMe device, a buggy firmware, or an attacker-controlled
NVMe-oF target.

The check is performed before queue_limits_start_update() and
blk_mq_freeze_queue(), so the error path is a plain `goto out` with
no cleanup needed.

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.

Acked-by: Sungwoo Kim <iam@sung-woo.kim>
Acked-by: Dave Tian <daveti@purdue.edu>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---

Changes since v2:
- Hoist the validation above queue_limits_start_update() and
  blk_mq_freeze_queue(); error path is now a plain `goto out`.
- Merge the ds == 0 case into the general invalid check.

v1: https://lore.kernel.org/linux-nvme/20260418042835.420281-1-coshi036@gmail.com/
v2: https://lore.kernel.org/linux-nvme/20260420231116.748204-2-coshi036@gmail.com/

 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 1e33af94c24b..12ff562dd142 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2404,12 +2404,22 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 			goto out;
 	}
 
+	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);
+		ret = -ENODEV;
+		goto out;
+	}
+
 	lim = queue_limits_start_update(ns->disk->queue);
 
 	memflags = blk_mq_freeze_queue(ns->disk->queue);
 	ns->head->lba_shift = id->lbaf[lbaf].ds;
 	ns->head->nuse = le64_to_cpu(id->nuse);
-	capacity = nvme_lba_to_sect(ns->head, le64_to_cpu(id->nsze));
 	nvme_set_ctrl_limits(ns->ctrl, &lim, false);
 	nvme_configure_metadata(ns->ctrl, ns->head, id, nvm, info);
 	nvme_set_chunk_sectors(ns, id, &lim);
-- 
2.43.0

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

[Keith Busch]

On Fri, May 15, 2026 at 02:58:53PM -0400, 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
> -ENODEV instead of crashing the kernel.  This is reachable by a
> malicious NVMe device, a buggy firmware, or an attacker-controlled
> NVMe-oF target.

Thanks, applied to nvme-7.2.

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

[John Garry]

On 15/05/2026 19:58, Chao Shi wrote:
>   
> +	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);
> +		ret = -ENODEV;
> +		goto out;
> +	}

JFYI, this is giving a C=1 warning:

drivers/nvme/host/core.c:2411:13: warning: unsigned value that used to
be signed checked against zero?
drivers/nvme/host/core.c:2411:13: signed value source

I can't seem to quieten it myself, though.

BTW, I would have thought that check_shl_overflow would catch 
id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).

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

[Keith Busch]

On Tue, Jun 02, 2026 at 02:10:07PM +0100, John Garry wrote:
> On 15/05/2026 19:58, Chao Shi wrote:
> > +	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);
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> 
> JFYI, this is giving a C=1 warning:
> 
> drivers/nvme/host/core.c:2411:13: warning: unsigned value that used to  be signed checked against zero?
> drivers/nvme/host/core.c:2411:13: signed value source
> 
> I can't seem to quieten it myself, though.
> 
> BTW, I would have thought that check_shl_overflow would catch
> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).

I see it too. check_shl_overflow has checks that suggest it was
expecting a signed type, as all the < 0 checks don't make sense for
unsigned. The warning seems harmless, but I'd too like to see it
suppressed.

I think it's odd that I'm not seeing a similar error for the similar
usage in generic_check_addressable() from fs/libfs.c. They look the same
to me with respect to the types passed in.

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

[Keith Busch]

On Tue, Jun 02, 2026 at 04:15:41PM +0100, Keith Busch wrote:
> On Tue, Jun 02, 2026 at 02:10:07PM +0100, John Garry wrote:
> > On 15/05/2026 19:58, Chao Shi wrote:
> > > +	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);
> > > +		ret = -ENODEV;
> > > +		goto out;
> > > +	}
> > 
> > JFYI, this is giving a C=1 warning:
> > 
> > drivers/nvme/host/core.c:2411:13: warning: unsigned value that used to  be signed checked against zero?
> > drivers/nvme/host/core.c:2411:13: signed value source
> > 
> > I can't seem to quieten it myself, though.
> > 
> > BTW, I would have thought that check_shl_overflow would catch
> > id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> 
> I see it too. check_shl_overflow has checks that suggest it was
> expecting a signed type, as all the < 0 checks don't make sense for
> unsigned. The warning seems harmless, but I'd too like to see it
> suppressed.
> 
> I think it's odd that I'm not seeing a similar error for the similar
> usage in generic_check_addressable() from fs/libfs.c. They look the same
> to me with respect to the types passed in.

It appears that sparse is having trouble with the type provenance of a
__bitwise __le64 type. No idea why. As a test, I replaced the
le64_to_cpu() to a u64 type on stack initialized to a random ULL value
and the warning goes away. I say we can ignore the sparse warning, or we
can rewrite this to avoid the check_shl_overflow entirely.

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cad9d97352615..6409a8218e3eb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2372,8 +2372,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	struct nvme_zone_info zi = {};
 	struct nvme_id_ns *id;
 	unsigned int memflags;
-	sector_t capacity;
-	unsigned lbaf;
+	unsigned lbaf, shift = 0;
+	u64 capacity, nsze;
 	int ret;
 
 	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
@@ -2407,10 +2407,13 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 			goto out;
 	}
 
-	if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
-	    check_shl_overflow(le64_to_cpu(id->nsze),
-			       id->lbaf[lbaf].ds - SECTOR_SHIFT,
-			       &capacity)) {
+	nsze = le64_to_cpu(id->nsze);
+	if (id->lbaf[lbaf].ds >= SECTOR_SHIFT)
+		shift = id->lbaf[lbaf].ds - SECTOR_SHIFT;
+
+	if (shift < SECTOR_SHIFT || shift >= 64 || nsze > U64_MAX >> shift) {
 		dev_warn_once(ns->ctrl->device,
 			"invalid LBA data size %u, skipping namespace\n",
 			id->lbaf[lbaf].ds);
--

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

[John Garry]

On 02/06/2026 16:42, Keith Busch wrote:
> On Tue, Jun 02, 2026 at 04:15:41PM +0100, Keith Busch wrote:
>> On Tue, Jun 02, 2026 at 02:10:07PM +0100, John Garry wrote:
>>> On 15/05/2026 19:58, Chao Shi wrote:
>>>> +	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);
>>>> +		ret = -ENODEV;
>>>> +		goto out;
>>>> +	}
>>>
>>> JFYI, this is giving a C=1 warning:
>>>
>>> drivers/nvme/host/core.c:2411:13: warning: unsigned value that used to  be signed checked against zero?
>>> drivers/nvme/host/core.c:2411:13: signed value source
>>>
>>> I can't seem to quieten it myself, though.
>>>
>>> BTW, I would have thought that check_shl_overflow would catch
>>> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
>>
>> I see it too. check_shl_overflow has checks that suggest it was
>> expecting a signed type, as all the < 0 checks don't make sense for
>> unsigned. The warning seems harmless, but I'd too like to see it
>> suppressed.
>>
>> I think it's odd that I'm not seeing a similar error for the similar
>> usage in generic_check_addressable() from fs/libfs.c. They look the same
>> to me with respect to the types passed in.
> 
> It appears that sparse is having trouble with the type provenance of a
> __bitwise __le64 type. No idea why. As a test, I replaced the
> le64_to_cpu() to a u64 type on stack initialized to a random ULL value
> and the warning goes away.

Yeah, ditto.

> I say we can ignore the sparse warning, or we
> can rewrite this to avoid the check_shl_overflow entirely.

Since sparse is having problems with le64_to_cpu(), I suppose using your 
own version is ok. It would be nice to know the root cause of this 
issue, though...

cheers

> 
> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cad9d97352615..6409a8218e3eb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2372,8 +2372,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   	struct nvme_zone_info zi = {};
>   	struct nvme_id_ns *id;
>   	unsigned int memflags;
> -	sector_t capacity;
> -	unsigned lbaf;
> +	unsigned lbaf, shift = 0;
> +	u64 capacity, nsze;
>   	int ret;
>   
>   	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
> @@ -2407,10 +2407,13 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   			goto out;
>   	}
>   
> -	if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> -	    check_shl_overflow(le64_to_cpu(id->nsze),
> -			       id->lbaf[lbaf].ds - SECTOR_SHIFT,
> -			       &capacity)) {
> +	nsze = le64_to_cpu(id->nsze);
> +	if (id->lbaf[lbaf].ds >= SECTOR_SHIFT)
> +		shift = id->lbaf[lbaf].ds - SECTOR_SHIFT;
> +
> +	if (shift < SECTOR_SHIFT || shift >= 64 || nsze > U64_MAX >> shift) {
>   		dev_warn_once(ns->ctrl->device,
>   			"invalid LBA data size %u, skipping namespace\n",
>   			id->lbaf[lbaf].ds);



> --

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

[John Garry]

On 02/06/2026 17:18, John Garry wrote:
> 
>> I say we can ignore the sparse warning, or we
>> can rewrite this to avoid the check_shl_overflow entirely.
> 

FWIW, adding a separate function keeps sparse happy for me:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ea837b94d3e5..3ec98038668e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2364,6 +2364,11 @@ static int nvme_query_fdp_info(struct nvme_ns 
*ns, struct nvme_ns_info *info)
  	return ret;
  }

+static bool nvme_valid_ds(u64 nsze, signed int shift, u64 *capacity)
+{
+	return check_shl_overflow(nsze, shift, capacity);
+}
+
  static int nvme_update_ns_info_block(struct nvme_ns *ns,
  		struct nvme_ns_info *info)
  {
@@ -2407,10 +2412,8 @@ static int nvme_update_ns_info_block(struct 
nvme_ns *ns,
  			goto out;
  	}

-	if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
-	    check_shl_overflow(le64_to_cpu(id->nsze),
-			       id->lbaf[lbaf].ds - SECTOR_SHIFT,
-			       &capacity)) {
+	if (nvme_valid_ds(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);

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

[Chao S]

Hi John, Keith,

Thanks for catching the sparse warning, and sorry for the delayed response.

> BTW, I have thought that check_shl_overflow would catch
> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).

Confirmed -- check_shl_overflow() returns true for the negative shift
that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
_to_shift != _s test fires). I checked ds=0 and ds=8: both are still
rejected with the explicit lower-bound test removed, so it is redundant.

For the C=1 warning, the minimal fix is to drop that redundant check and
feed nsze through a plain u64 local -- as Keith found, laundering the
le64_to_cpu() result through a non-__le64 type makes the warning go away:

u64 nsze;
...
nsze = le64_to_cpu(id->nsze);
if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
      &capacity)) {
dev_warn_once(...);
ret = -ENODEV;
goto out;
}

This keeps check_shl_overflow() in one tested helper and avoids a
wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
for its actual sense (it returns true on overflow, i.e. invalid), e.g.
nvme_ds_overflows().

One note: I'd lean toward keeping check_shl_overflow() rather than
open-coding the bound. It folds the lower-bound (negative shift) and the
overflow case into one tested helper, so we don't have to re-derive the
boundary by hand -- e.g. the lower bound is on ds itself, not on the
post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
subtly wrong.

Keith, since v3 is already in your tree: do you want an incremental fixup
on top, or a v4 to replace the applied commit? I have both ready.

Thanks,
Chao

On Wed, Jun 3, 2026 at 6:08 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 02/06/2026 17:18, John Garry wrote:
> >
> >> I say we can ignore the sparse warning, or we
> >> can rewrite this to avoid the check_shl_overflow entirely.
> >
>
> FWIW, adding a separate function keeps sparse happy for me:
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ea837b94d3e5..3ec98038668e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2364,6 +2364,11 @@ static int nvme_query_fdp_info(struct nvme_ns
> *ns, struct nvme_ns_info *info)
>         return ret;
>   }
>
> +static bool nvme_valid_ds(u64 nsze, signed int shift, u64 *capacity)
> +{
> +       return check_shl_overflow(nsze, shift, capacity);
> +}
> +
>   static int nvme_update_ns_info_block(struct nvme_ns *ns,
>                 struct nvme_ns_info *info)
>   {
> @@ -2407,10 +2412,8 @@ static int nvme_update_ns_info_block(struct
> nvme_ns *ns,
>                         goto out;
>         }
>
> -       if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> -           check_shl_overflow(le64_to_cpu(id->nsze),
> -                              id->lbaf[lbaf].ds - SECTOR_SHIFT,
> -                              &capacity)) {
> +       if (nvme_valid_ds(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);
>

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

[John Garry]

On 23/06/2026 21:37, Chao S wrote:
>> BTW, I have thought that check_shl_overflow would catch
>> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> Confirmed -- check_shl_overflow() returns true for the negative shift
> that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
> _to_shift != _s test fires). I checked ds=0 and ds=8: both are still
> rejected with the explicit lower-bound test removed, so it is redundant.
> 
> For the C=1 warning, the minimal fix is to drop that redundant check and
> feed nsze through a plain u64 local -- as Keith found, laundering the
> le64_to_cpu() result through a non-__le64 type makes the warning go away:
> 
> u64 nsze;
> ...
> nsze = le64_to_cpu(id->nsze);
> if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
>        &capacity)) {
> dev_warn_once(...);
> ret = -ENODEV;
> goto out;
> }
> 
> This keeps check_shl_overflow() in one tested helper and avoids a
> wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
> for its actual sense (it returns true on overflow, i.e. invalid), e.g.
> nvme_ds_overflows().
> 
> One note: I'd lean toward keeping check_shl_overflow() rather than
> open-coding the bound. It folds the lower-bound (negative shift) and the
> overflow case into one tested helper, so we don't have to re-derive the
> boundary by hand -- e.g. the lower bound is on ds itself, not on the
> post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
> subtly wrong.

What exactly is your proposed change (to what is in the tree)?

> 
> Keith, since v3 is already in your tree: do you want an incremental fixup
> on top, or a v4 to replace the applied commit? I have both ready.
> 

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

[Chao S]

Hi John,

Here is the concrete change (drivers/nvme/host/core.c,
nvme_update_ns_info_block()):

  unsigned int memflags;
  sector_t capacity;
  unsigned lbaf;
+ u64 nsze;
  int ret;
...
- if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
-    check_shl_overflow(le64_to_cpu(id->nsze),
-       id->lbaf[lbaf].ds - SECTOR_SHIFT,
-       &capacity)) {
+ /*
+ * check_shl_overflow() also rejects a data size below SECTOR_SHIFT,
+ * as that makes the shift count negative, so no separate lower-bound
+ * test is needed.  Feed nsze through a plain u64 so sparse does not
+ * trip over the __le64 provenance of le64_to_cpu().
+ */
+ nsze = le64_to_cpu(id->nsze);
+ if (check_shl_overflow(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);
  ret = -ENODEV;
  goto out;
  }

Two things:

1. Drops the explicit ds < SECTOR_SHIFT test as redundant --
   check_shl_overflow() already returns true for the resulting negative
   shift. I confirmed ds=0 and ds=8 are still rejected without it.

2. Routes le64_to_cpu(id->nsze) through a plain u64 local, which is what
   silences the C=1 warning (sparse loses the __le64 provenance when the
   value is passed straight into check_shl_overflow()).

Behavior is otherwise unchanged. Since the original is already in
v7.2-rc1, I'll send this as a standalone patch on top of mainline
(Reported-by: you) unless you'd prefer a different form.

Thanks,
Chao

On Wed, Jun 24, 2026 at 9:15 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 23/06/2026 21:37, Chao S wrote:
> >> BTW, I have thought that check_shl_overflow would catch
> >> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> > Confirmed -- check_shl_overflow() returns true for the negative shift
> > that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
> > _to_shift != _s test fires). I checked ds=0 and ds=8: both are still
> > rejected with the explicit lower-bound test removed, so it is redundant.
> >
> > For the C=1 warning, the minimal fix is to drop that redundant check and
> > feed nsze through a plain u64 local -- as Keith found, laundering the
> > le64_to_cpu() result through a non-__le64 type makes the warning go away:
> >
> > u64 nsze;
> > ...
> > nsze = le64_to_cpu(id->nsze);
> > if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
> >        &capacity)) {
> > dev_warn_once(...);
> > ret = -ENODEV;
> > goto out;
> > }
> >
> > This keeps check_shl_overflow() in one tested helper and avoids a
> > wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
> > for its actual sense (it returns true on overflow, i.e. invalid), e.g.
> > nvme_ds_overflows().
> >
> > One note: I'd lean toward keeping check_shl_overflow() rather than
> > open-coding the bound. It folds the lower-bound (negative shift) and the
> > overflow case into one tested helper, so we don't have to re-derive the
> > boundary by hand -- e.g. the lower bound is on ds itself, not on the
> > post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
> > subtly wrong.
>
> What exactly is your proposed change (to what is in the tree)?
>
> >
> > Keith, since v3 is already in your tree: do you want an incremental fixup
> > on top, or a v4 to replace the applied commit? I have both ready.
> >
>
>
>

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

[John Garry]

This response took almost 3 weeks. The previous response took again 
almost 3 weeks. kernel development may be relatively slow moving, but 
it's not that slow. You need to respond much more promptly to keep up 
with current development.

> 
> Here is the concrete change (drivers/nvme/host/core.c,
> nvme_update_ns_info_block()):
> 
>    unsigned int memflags;
>    sector_t capacity;
>    unsigned lbaf;
> + u64 nsze;
>    int ret;
> ...
> - if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> -    check_shl_overflow(le64_to_cpu(id->nsze),
> -       id->lbaf[lbaf].ds - SECTOR_SHIFT,
> -       &capacity)) {
> + /*
> + * check_shl_overflow() also rejects a data size below SECTOR_SHIFT,
> + * as that makes the shift count negative, so no separate lower-bound
> + * test is needed.  Feed nsze through a plain u64 so sparse does not
> + * trip over the __le64 provenance of le64_to_cpu().
> + */
> + nsze = le64_to_cpu(id->nsze);
> + if (check_shl_overflow(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);
>    ret = -ENODEV;
>    goto out;
>    }
> 
> Two things:
> 
> 1. Drops the explicit ds < SECTOR_SHIFT test as redundant --
>     check_shl_overflow() already returns true for the resulting negative
>     shift. I confirmed ds=0 and ds=8 are still rejected without it.
> 
> 2. Routes le64_to_cpu(id->nsze) through a plain u64 local, which is what
>     silences the C=1 warning (sparse loses the __le64 provenance when the
>     value is passed straight into check_shl_overflow()).
> 
> Behavior is otherwise unchanged. Since the original is already in
> v7.2-rc1, I'll send this as a standalone patch on top of mainline
> (Reported-by: you) unless you'd prefer a different form.

My fix is now in mainline 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v7.2-rc3&id=92f58587a04c94985fd4a9e3575720b054c432bf, 
so any change which you want to make must be on top of that.

> 
> Thanks,
> Chao
> 
> On Wed, Jun 24, 2026 at 9:15 AM John Garry <john.g.garry@oracle.com> wrote:
>>
>> On 23/06/2026 21:37, Chao S wrote:
>>>> BTW, I have thought that check_shl_overflow would catch
>>>> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
>>> Confirmed -- check_shl_overflow() returns true for the negative shift
>>> that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
>>> _to_shift != _s test fires). I checked ds=0 and ds=8: both are still
>>> rejected with the explicit lower-bound test removed, so it is redundant.
>>>
>>> For the C=1 warning, the minimal fix is to drop that redundant check and
>>> feed nsze through a plain u64 local -- as Keith found, laundering the
>>> le64_to_cpu() result through a non-__le64 type makes the warning go away:
>>>
>>> u64 nsze;
>>> ...
>>> nsze = le64_to_cpu(id->nsze);
>>> if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
>>>         &capacity)) {
>>> dev_warn_once(...);
>>> ret = -ENODEV;
>>> goto out;
>>> }
>>>
>>> This keeps check_shl_overflow() in one tested helper and avoids a
>>> wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
>>> for its actual sense (it returns true on overflow, i.e. invalid), e.g.
>>> nvme_ds_overflows().
>>>
>>> One note: I'd lean toward keeping check_shl_overflow() rather than
>>> open-coding the bound. It folds the lower-bound (negative shift) and the
>>> overflow case into one tested helper, so we don't have to re-derive the
>>> boundary by hand -- e.g. the lower bound is on ds itself, not on the
>>> post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
>>> subtly wrong.
>>
>> What exactly is your proposed change (to what is in the tree)?
>>
>>>
>>> Keith, since v3 is already in your tree: do you want an incremental fixup
>>> on top, or a v4 to replace the applied commit? I have both ready.
>>>
>>
>>
>>

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

[Chao S]

Hi John,

I apologize for the delayed response. I may have missed the notifications,
since at the same time, we have some crashes and bug patches working
in parallel. So the response process is a little slower. Thanks for
letting me know
the newest fixup. Apologize again!

Best,
Chao

On Tue, Jul 14, 2026 at 4:18 AM John Garry <john.g.garry@oracle.com> wrote:
>
> This response took almost 3 weeks. The previous response took again
> almost 3 weeks. kernel development may be relatively slow moving, but
> it's not that slow. You need to respond much more promptly to keep up
> with current development.
>
> >
> > Here is the concrete change (drivers/nvme/host/core.c,
> > nvme_update_ns_info_block()):
> >
> >    unsigned int memflags;
> >    sector_t capacity;
> >    unsigned lbaf;
> > + u64 nsze;
> >    int ret;
> > ...
> > - if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> > -    check_shl_overflow(le64_to_cpu(id->nsze),
> > -       id->lbaf[lbaf].ds - SECTOR_SHIFT,
> > -       &capacity)) {
> > + /*
> > + * check_shl_overflow() also rejects a data size below SECTOR_SHIFT,
> > + * as that makes the shift count negative, so no separate lower-bound
> > + * test is needed.  Feed nsze through a plain u64 so sparse does not
> > + * trip over the __le64 provenance of le64_to_cpu().
> > + */
> > + nsze = le64_to_cpu(id->nsze);
> > + if (check_shl_overflow(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);
> >    ret = -ENODEV;
> >    goto out;
> >    }
> >
> > Two things:
> >
> > 1. Drops the explicit ds < SECTOR_SHIFT test as redundant --
> >     check_shl_overflow() already returns true for the resulting negative
> >     shift. I confirmed ds=0 and ds=8 are still rejected without it.
> >
> > 2. Routes le64_to_cpu(id->nsze) through a plain u64 local, which is what
> >     silences the C=1 warning (sparse loses the __le64 provenance when the
> >     value is passed straight into check_shl_overflow()).
> >
> > Behavior is otherwise unchanged. Since the original is already in
> > v7.2-rc1, I'll send this as a standalone patch on top of mainline
> > (Reported-by: you) unless you'd prefer a different form.
>
> My fix is now in mainline
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v7.2-rc3&id=92f58587a04c94985fd4a9e3575720b054c432bf,
> so any change which you want to make must be on top of that.
>
> >
> > Thanks,
> > Chao
> >
> > On Wed, Jun 24, 2026 at 9:15 AM John Garry <john.g.garry@oracle.com> wrote:
> >>
> >> On 23/06/2026 21:37, Chao S wrote:
> >>>> BTW, I have thought that check_shl_overflow would catch
> >>>> id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> >>> Confirmed -- check_shl_overflow() returns true for the negative shift
> >>> that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
> >>> _to_shift != _s test fires). I checked ds=0 and ds=8: both are still
> >>> rejected with the explicit lower-bound test removed, so it is redundant.
> >>>
> >>> For the C=1 warning, the minimal fix is to drop that redundant check and
> >>> feed nsze through a plain u64 local -- as Keith found, laundering the
> >>> le64_to_cpu() result through a non-__le64 type makes the warning go away:
> >>>
> >>> u64 nsze;
> >>> ...
> >>> nsze = le64_to_cpu(id->nsze);
> >>> if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
> >>>         &capacity)) {
> >>> dev_warn_once(...);
> >>> ret = -ENODEV;
> >>> goto out;
> >>> }
> >>>
> >>> This keeps check_shl_overflow() in one tested helper and avoids a
> >>> wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
> >>> for its actual sense (it returns true on overflow, i.e. invalid), e.g.
> >>> nvme_ds_overflows().
> >>>
> >>> One note: I'd lean toward keeping check_shl_overflow() rather than
> >>> open-coding the bound. It folds the lower-bound (negative shift) and the
> >>> overflow case into one tested helper, so we don't have to re-derive the
> >>> boundary by hand -- e.g. the lower bound is on ds itself, not on the
> >>> post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
> >>> subtly wrong.
> >>
> >> What exactly is your proposed change (to what is in the tree)?
> >>
> >>>
> >>> Keith, since v3 is already in your tree: do you want an incremental fixup
> >>> on top, or a v4 to replace the applied commit? I have both ready.
> >>>
> >>
> >>
> >>
>