Skip to contentSkip to conversation

nvme: fix string_matches() helper

PATCH New Christoph Hellwig 3 participants 5 messages Jun 13, 2017 – Jun 15, 2017 NVMe

lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

PATCH 1/1 nvme: fix string_matches() helper

NVMe "ASCII" strings are not nul-terminated and can use up every single byte in the field. Thus use strnlen to determine the match length instead of possibly overrunning the field.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/nvme/host/core.c +1 -3
index 434b18863895..2658a3a3cbb5 100644
@@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
 	if (!match)
 		return true;
 
-	matchlen = strlen(match);
-	WARN_ON_ONCE(matchlen > len);
-
+	matchlen = strnlen(match, len);
 	if (memcmp(idstr, match, matchlen))
 		return false;
Signature · 1 line
2.11.0
#1 lore raw

PATCH 1/1 nvme: fix string_matches() helper

Quoted 14 lines · Christoph Hellwig

NVMe "ASCII" strings are not nul-terminated and can use up every single byte in the field. Thus use strnlen to determine the match length instead of possibly overrunning the field.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/nvme/host/core.c +0 -0
index 434b18863895..2658a3a3cbb5 100644

drivers/nvme/host/core.c static bool string_matches(const char *idstr, const char *match, size_t len) · line 1584

Quoted 7 lines
drivers/nvme/host/core.c +1 -3
@@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
        if (!match)
                return true;
 
-       matchlen = strlen(match);
-       WARN_ON_ONCE(matchlen > len);
-
+       matchlen = strnlen(match, len);

"match" refers to the string in the quirk table, which should be a plain C string. Are you hitting this in practice?

author maintainer
#2 lore raw

PATCH 1/1 nvme: fix string_matches() helper

Quoted 18 lines · Andy Lutomirski
Quoted 17 lines · Christoph Hellwig

NVMe "ASCII" strings are not nul-terminated and can use up every single byte in the field. Thus use strnlen to determine the match length instead of possibly overrunning the field.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/nvme/host/core.c +0 -0
index 434b18863895..2658a3a3cbb5 100644
@@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
        if (!match)
                return true;
Quoted 7 lines

drivers/nvme/host/core.c static bool string_matches(const char *idstr, const char *match, size_t len) · line 1584

Quoted 4 lines
drivers/nvme/host/core.c +1 -3
-       matchlen = strlen(match);
-       WARN_ON_ONCE(matchlen > len);
-
+       matchlen = strnlen(match, len);

"match" refers to the string in the quirk table, which should be a plain C string. Are you hitting this in practice?

No, but I've just written some other code that deals with NVMe strings, and took extra care of bounds protection.

But yes, given the strlen is on the match we should be ok as long as the WARN_ON_ONCE also does an early return. Does that sound ok to you?

#3 lore raw

PATCH 1/1 nvme: fix string_matches() helper

Quoted 27 lines · Christoph Hellwig
Quoted 18 lines · Andy Lutomirski
Quoted 17 lines · Christoph Hellwig

NVMe "ASCII" strings are not nul-terminated and can use up every single byte in the field. Thus use strnlen to determine the match length instead of possibly overrunning the field.

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/nvme/host/core.c +0 -0
index 434b18863895..2658a3a3cbb5 100644
@@ -1581,9 +1581,7 @@ static bool string_matches(const char *idstr, const char *match, size_t len)
        if (!match)
                return true;
Quoted 7 lines

drivers/nvme/host/core.c static bool string_matches(const char *idstr, const char *match, size_t len) · line 1584

Quoted 4 lines
drivers/nvme/host/core.c +1 -3
-       matchlen = strlen(match);
-       WARN_ON_ONCE(matchlen > len);
-
+       matchlen = strnlen(match, len);

"match" refers to the string in the quirk table, which should be a plain C string. Are you hitting this in practice?

Quoted 6 lines

No, but I've just written some other code that deals with NVMe strings, and took extra care of bounds protection.

But yes, given the strlen is on the match we should be ok as long as the WARN_ON_ONCE also does an early return. Does that sound ok to you?

Sounds good to me.