[PATCH] nvme: nvme_identify_ns_descs: prevent oob

[PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Eugene Korenevsky]

Broken or malicious controller can send invalid ns id.
Out-of-band memory access may occur if remaining buffer size
is less than .nidl (ns id length) field of `struct nvme_ns_id_desc`

Fix this issue by making nvme_process_id_decs() function aware of
remaining buffer size.

Also simplify nvme_process_id_decs(): replace copy-pasted `case`
branches with table lookup.

Signed-off-by: Eugene Korenevsky <ekorenevsky@aliyun.com>
---
 drivers/nvme/host/core.c | 80 ++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 41 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f1f719351f3f..2d1a4b9882ca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1481,52 +1481,45 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 	return error;
 }
 
+struct ns_id_info {
+	__u8 nidl;
+	char *name;
+};
+
+#define NS_ID_INFO(nidt) \
+	[nidt] = {nidt##_LEN, #nidt}
+
+static const struct ns_id_info niis[] = {
+	NS_ID_INFO(NVME_NIDT_EUI64),
+	NS_ID_INFO(NVME_NIDT_NGUID),
+	NS_ID_INFO(NVME_NIDT_UUID),
+	NS_ID_INFO(NVME_NIDT_CSI),
+};
+
 static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
-		struct nvme_ns_id_desc *cur, bool *csi_seen)
+				struct nvme_ns_id_desc *cur, int remain,
+				bool *csi_seen)
 {
-	const char *warn_str = "ctrl returned bogus length:";
-	void *data = cur;
+	const char *warn_fmt = "ctrl returned bogus ns id: %u %u %d %s";
+	const struct ns_id_info *nii;
 
 	switch (cur->nidt) {
+	case NVME_NIDT_CSI:
+		*csi_seen = true;
+		fallthrough;
 	case NVME_NIDT_EUI64:
-		if (cur->nidl != NVME_NIDT_EUI64_LEN) {
-			dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n",
-				 warn_str, cur->nidl);
-			return -1;
-		}
-		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
-			return NVME_NIDT_EUI64_LEN;
-		memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
-		return NVME_NIDT_EUI64_LEN;
 	case NVME_NIDT_NGUID:
-		if (cur->nidl != NVME_NIDT_NGUID_LEN) {
-			dev_warn(ctrl->device, "%s %d for NVME_NIDT_NGUID\n",
-				 warn_str, cur->nidl);
-			return -1;
-		}
-		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
-			return NVME_NIDT_NGUID_LEN;
-		memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
-		return NVME_NIDT_NGUID_LEN;
 	case NVME_NIDT_UUID:
-		if (cur->nidl != NVME_NIDT_UUID_LEN) {
-			dev_warn(ctrl->device, "%s %d for NVME_NIDT_UUID\n",
-				 warn_str, cur->nidl);
+		nii = &niis[cur->nidt];
+		if (cur->nidl != nii->nidl || remain < nii->nidl) {
+			dev_warn(ctrl->device, warn_fmt,
+				 cur->nidl, nii->nidl, remain, nii->name);
 			return -1;
 		}
 		if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
-			return NVME_NIDT_UUID_LEN;
-		uuid_copy(&ids->uuid, data + sizeof(*cur));
-		return NVME_NIDT_UUID_LEN;
-	case NVME_NIDT_CSI:
-		if (cur->nidl != NVME_NIDT_CSI_LEN) {
-			dev_warn(ctrl->device, "%s %d for NVME_NIDT_CSI\n",
-				 warn_str, cur->nidl);
-			return -1;
-		}
-		memcpy(&ids->csi, data + sizeof(*cur), NVME_NIDT_CSI_LEN);
-		*csi_seen = true;
-		return NVME_NIDT_CSI_LEN;
+			return nii->nidl;
+		memcpy(&ids->csi, cur + 1, nii->nidl);
+		return nii->nidl;
 	default:
 		/* Skip unknown types */
 		return cur->nidl;
@@ -1538,7 +1531,8 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
 {
 	struct nvme_command c = { };
 	bool csi_seen = false;
-	int status, pos, len;
+	int status, len, remain;
+	struct nvme_ns_id_desc *cur;
 	void *data;
 
 	if (ctrl->vs < NVME_VS(1, 3, 0) && !nvme_multi_css(ctrl))
@@ -1563,17 +1557,21 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
 		goto free_data;
 	}
 
-	for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
-		struct nvme_ns_id_desc *cur = data + pos;
-
+	remain = NVME_IDENTIFY_DATA_SIZE;
+	cur = data;
+	while (remain >= sizeof(*cur)) {
 		if (cur->nidl == 0)
 			break;
 
-		len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
+		len = nvme_process_ns_desc(ctrl, &info->ids,
+					   cur, remain - sizeof(*cur),
+					   &csi_seen);
 		if (len < 0)
 			break;
 
 		len += sizeof(*cur);
+		remain -= len;
+		cur += len;
 	}
 
 	if (nvme_multi_css(ctrl) && !csi_seen) {
-- 
2.47.3

Re: [PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Keith Busch]

On Wed, Nov 26, 2025 at 11:27:21PM +0300, Eugene Korenevsky wrote:
> Broken or malicious controller can send invalid ns id.
> Out-of-band memory access may occur if remaining buffer size
> is less than .nidl (ns id length) field of `struct nvme_ns_id_desc`
> 
> Fix this issue by making nvme_process_id_decs() function aware of
> remaining buffer size.
> 
> Also simplify nvme_process_id_decs(): replace copy-pasted `case`
> branches with table lookup.
> 
> Signed-off-by: Eugene Korenevsky <ekorenevsky@aliyun.com>
> ---
>  drivers/nvme/host/core.c | 80 ++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 41 deletions(-)

Is this simpler check not sufficient?

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fa534f1d6b27a..6cc43cbb04dd9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1566,7 +1566,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
        for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
                struct nvme_ns_id_desc *cur = data + pos;

-               if (cur->nidl == 0)
+               if (cur->nidl == 0 || cur->nidl + pos > NVME_IDENTIFY_DATA_SIZE)
                        break;

                len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
--

Re: [PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Eugene Korenevsky]

On Wed, Nov 26, 2025 at 01:52:08PM -0700, Keith Busch wrote:

> Is this simpler check not sufficient?

> @@ -1566,7 +1566,7 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl,
>         for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {

Alas size of ns id header+data may be 4+1=5 if .nidt == NVME_NIDT_CSI.
`pos` may be not aligned to 4 (size of `struct nvme_ns_id_desc` header).
It can be NVME_IDENTIFY_DATA_SIZE-1 here.

And when cur->nidl (i.e. pos+1) is referenced later, we'll get a problem.

>                 struct nvme_ns_id_desc *cur = data + pos;
> 
> -               if (cur->nidl == 0)
> +               if (cur->nidl == 0 || cur->nidl + pos > NVME_IDENTIFY_DATA_SIZE)
>                         break;
> 
>                 len = nvme_process_ns_desc(ctrl, &info->ids, cur, &csi_seen);
> --

Re: [PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Christoph Hellwig]

On Wed, Nov 26, 2025 at 01:52:08PM -0700, Keith Busch wrote:
> On Wed, Nov 26, 2025 at 11:27:21PM +0300, Eugene Korenevsky wrote:
> > Broken or malicious controller can send invalid ns id.
> > Out-of-band memory access may occur if remaining buffer size
> > is less than .nidl (ns id length) field of `struct nvme_ns_id_desc`
> > 
> > Fix this issue by making nvme_process_id_decs() function aware of
> > remaining buffer size.
> > 
> > Also simplify nvme_process_id_decs(): replace copy-pasted `case`
> > branches with table lookup.
> > 
> > Signed-off-by: Eugene Korenevsky <ekorenevsky@aliyun.com>
> > ---
> >  drivers/nvme/host/core.c | 80 ++++++++++++++++++++--------------------
> >  1 file changed, 39 insertions(+), 41 deletions(-)
> 
> Is this simpler check not sufficient?

I think we'll need a somewhat more complex check that at there
is at least enough space for the len member.  But I prefer that
style over a table lookup and magic name pasting macros.

Re: [PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Eugene Korenevsky]

On Thu, Nov 27, 2025 at 03:49:16PM +0100, Christoph Hellwig wrote:

> I think we'll need a somewhat more complex check that at there
> is at least enough space for the len member.  But I prefer that
> style over a table lookup and magic name pasting macros.

Okay, simplified the patch. See v2.

Re: [PATCH] nvme: nvme_identify_ns_descs: prevent oob

[Sagi Grimberg]

On 29/11/2025 21:57, Eugene Korenevsky wrote:
> On Thu, Nov 27, 2025 at 03:49:16PM +0100, Christoph Hellwig wrote:
>
>> I think we'll need a somewhat more complex check that at there
>> is at least enough space for the len member.  But I prefer that
>> style over a table lookup and magic name pasting macros.
> Okay, simplified the patch. See v2.

Did you send it?