linux-kernel - [PATCH] nvme-auth: fix input checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Sat, 16 Jul 2022 16:39:24 -0400
From:   Tom Rix <trix@...hat.com>
To:     nathan@...nel.org, ndesaulniers@...gle.com, sagi@...mberg.me,
        hare@...e.de
Cc:     linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        Tom Rix <trix@...hat.com>
Subject: [PATCH] nvme-auth: fix input checking

clang build fails with this representative error
drivers/nvme/common/auth.c:59:31: error: address of array 'dhgroup_map[dhgroup_id].name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
            !dhgroup_map[dhgroup_id].name ||
            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

Input is checked with this pattern
  if (i > ARRAY_SIZE(a))
    fail
  return a[i].x

This is an off-by-one bug.  Change to
  if (i < ARRAY_SIZE(a)
    return a[i].x
  return fail

By specifying the 'name' element of the nvme_auth_dhgroup_map struct as
a constant, pre sized array, it will be never be NULL.  So this and similar
pointer checks are not needed.

By inspection, none of the strings are zero length.  So the strlen() check
is also not needed.

The array dhgroup_map[] is unchanging so it should have a const type
qualifier.

The hash_map[] array has an additional problem.  The zeroth element of the
array is not explicitly initialized so it is implicitly zero initialized.
The pointer will be valid but be empty strings.  Instead of calling strlen(),
check for the zeroth element.

Fixes: a476416bb57b ("nvme: implement In-Band authentication")
Signed-off-by: Tom Rix <trix@...hat.com>
---
 drivers/nvme/common/auth.c | 54 +++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 33 deletions(-)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 0c86ebce59d2..b7f7ab37786c 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -35,7 +35,7 @@ u32 nvme_auth_get_seqnum(void)
 }
 EXPORT_SYMBOL_GPL(nvme_auth_get_seqnum);
 
-static struct nvme_auth_dhgroup_map {
+static const struct nvme_auth_dhgroup_map {
 	const char name[16];
 	const char kpp[16];
 } dhgroup_map[] = {
@@ -55,21 +55,17 @@ static struct nvme_auth_dhgroup_map {
 
 const char *nvme_auth_dhgroup_name(u8 dhgroup_id)
 {
-	if ((dhgroup_id > ARRAY_SIZE(dhgroup_map)) ||
-	    !dhgroup_map[dhgroup_id].name ||
-	    !strlen(dhgroup_map[dhgroup_id].name))
-		return NULL;
-	return dhgroup_map[dhgroup_id].name;
+	if (dhgroup_id < ARRAY_SIZE(dhgroup_map))
+		return dhgroup_map[dhgroup_id].name;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_dhgroup_name);
 
 const char *nvme_auth_dhgroup_kpp(u8 dhgroup_id)
 {
-	if ((dhgroup_id > ARRAY_SIZE(dhgroup_map)) ||
-	    !dhgroup_map[dhgroup_id].kpp ||
-	    !strlen(dhgroup_map[dhgroup_id].kpp))
-		return NULL;
-	return dhgroup_map[dhgroup_id].kpp;
+	if (dhgroup_id < ARRAY_SIZE(dhgroup_map))
+		return dhgroup_map[dhgroup_id].kpp;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_dhgroup_kpp);
 
@@ -78,9 +74,6 @@ u8 nvme_auth_dhgroup_id(const char *dhgroup_name)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(dhgroup_map); i++) {
-		if (!dhgroup_map[i].name ||
-		    !strlen(dhgroup_map[i].name))
-			continue;
 		if (!strncmp(dhgroup_map[i].name, dhgroup_name,
 			     strlen(dhgroup_map[i].name)))
 			return i;
@@ -89,7 +82,7 @@ u8 nvme_auth_dhgroup_id(const char *dhgroup_name)
 }
 EXPORT_SYMBOL_GPL(nvme_auth_dhgroup_id);
 
-static struct nvme_dhchap_hash_map {
+static const struct nvme_dhchap_hash_map {
 	int len;
 	const char hmac[15];
 	const char digest[8];
@@ -113,21 +106,19 @@ static struct nvme_dhchap_hash_map {
 
 const char *nvme_auth_hmac_name(u8 hmac_id)
 {
-	if ((hmac_id > ARRAY_SIZE(hash_map)) ||
-	    !hash_map[hmac_id].hmac ||
-	    !strlen(hash_map[hmac_id].hmac))
-		return NULL;
-	return hash_map[hmac_id].hmac;
+	if ((hmac_id > 0) &&
+	    (hmac_id < ARRAY_SIZE(hash_map)))
+		return hash_map[hmac_id].hmac;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_hmac_name);
 
 const char *nvme_auth_digest_name(u8 hmac_id)
 {
-	if ((hmac_id > ARRAY_SIZE(hash_map)) ||
-	    !hash_map[hmac_id].digest ||
-	    !strlen(hash_map[hmac_id].digest))
-		return NULL;
-	return hash_map[hmac_id].digest;
+	if ((hmac_id > 0) &&
+	    (hmac_id < ARRAY_SIZE(hash_map)))
+		return hash_map[hmac_id].digest;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_digest_name);
 
@@ -135,9 +126,7 @@ u8 nvme_auth_hmac_id(const char *hmac_name)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(hash_map); i++) {
-		if (!hash_map[i].hmac || !strlen(hash_map[i].hmac))
-			continue;
+	for (i = 1; i < ARRAY_SIZE(hash_map); i++) {
 		if (!strncmp(hash_map[i].hmac, hmac_name,
 			     strlen(hash_map[i].hmac)))
 			return i;
@@ -148,11 +137,10 @@ EXPORT_SYMBOL_GPL(nvme_auth_hmac_id);
 
 size_t nvme_auth_hmac_hash_len(u8 hmac_id)
 {
-	if ((hmac_id > ARRAY_SIZE(hash_map)) ||
-	    !hash_map[hmac_id].hmac ||
-	    !strlen(hash_map[hmac_id].hmac))
-		return 0;
-	return hash_map[hmac_id].len;
+	if ((hmac_id > 0) &&
+	    (hmac_id < ARRAY_SIZE(hash_map)))
+		return hash_map[hmac_id].len;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(nvme_auth_hmac_hash_len);
 
-- 
2.27.0