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

[Keith Busch <kbusch@kernel.org>]

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);
--