drivers/nvme/host/core.c | 80 ++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 41 deletions(-)
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.3On 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);
--
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.
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.
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?
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);
> --
© 2016 - 2025 Red Hat, Inc.