linux-kernel - [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

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>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Mar 2018 14:21:39 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Paul Blakey <paulb@...lanox.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Martin Sebor <msebor@....gnu.org>,
        Florian Westphal <fw@...len.de>, Phil Sutter <phil@....cc>,
        Tom Herbert <tom@...ntonium.net>, linux-kernel@...r.kernel.org
Subject: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

gcc-8 warns about a code pattern that is used in the newly added
test_rhashtable code:

lib/test_rhashtable.c: In function 'print_ht':
lib/test_rhashtable.c:511:21: error: '
bucket[' directive writing 8 bytes into a region of size between 1 and 512 [-Werror=format-overflow=]
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
                     ^~~~~~~~~
lib/test_rhashtable.c:511:4: note: 'sprintf' output between 15 and 536 bytes into a destination of size 512
    sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem here is using the same fixed-length buffer as input and output
of snprintf(), which for an unbounded loop has an actual potential to
overflow the buffer. The '512' byte length was apparently chosen to
be "long enough" to prevent that in practice, but without any specific
guarantees of being the smallest safe size.

I can see three possible ways to avoid this warning:

- rewrite the code to use pointer arithmetic to forward the buffer,
  rather than copying the buffer itself. This is a more conventional
  use of sprintf(), and it avoids the warning, but is not any more
  safe than the original code.
- Rewrite the function in a safe way that avoids both the potential
  overflow and the warning.
- Ask the gcc developers to not warn for this pattern if we consider
  the warning to be inappropriate.

This patch implements the first of the above, as an illustration of
the problem, and the simplest workaround.

Fixes: 499ac3b60f65 ("test_rhashtable: add test case for rhltable with duplicate objects")
Cc: Martin Sebor <msebor@....gnu.org>
Signed-off-by: Arnd Bergmann <arnd@...db.de>
---
My patch is untested, please try it out before applying.
---
 lib/test_rhashtable.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/test_rhashtable.c b/lib/test_rhashtable.c
index f4000c137dbe..a0f4fb03d2de 100644
--- a/lib/test_rhashtable.c
+++ b/lib/test_rhashtable.c
@@ -496,6 +496,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 	struct rhashtable *ht;
 	const struct bucket_table *tbl;
 	char buff[512] = "";
+	char *buffp = buff;
 	unsigned int i, cnt = 0;
 
 	ht = &rhlt->ht;
@@ -508,18 +509,18 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 		next = !rht_is_a_nulls(pos) ? rht_dereference(pos->next, ht) : NULL;
 
 		if (!rht_is_a_nulls(pos)) {
-			sprintf(buff, "%s\nbucket[%d] -> ", buff, i);
+			buffp += sprintf(buffp, "\nbucket[%d] -> ", i);
 		}
 
 		while (!rht_is_a_nulls(pos)) {
 			struct rhlist_head *list = container_of(pos, struct rhlist_head, rhead);
-			sprintf(buff, "%s[[", buff);
+			buffp += sprintf(buffp, "[[");
 			do {
 				pos = &list->rhead;
 				list = rht_dereference(list->next, ht);
 				p = rht_obj(ht, pos);
 
-				sprintf(buff, "%s val %d (tid=%d)%s", buff, p->value.id, p->value.tid,
+				buffp += sprintf(buffp, "val %d (tid=%d)%s", p->value.id, p->value.tid,
 					list? ", " : " ");
 				cnt++;
 			} while (list);
@@ -528,7 +529,7 @@ static unsigned int __init print_ht(struct rhltable *rhlt)
 			next = !rht_is_a_nulls(pos) ?
 				rht_dereference(pos->next, ht) : NULL;
 
-			sprintf(buff, "%s]]%s", buff, !rht_is_a_nulls(pos) ? " -> " : "");
+			buffp += sprintf(buffp, "]]%s", !rht_is_a_nulls(pos) ? " -> " : "");
 		}
 	}
 	printk(KERN_ERR "\n---- ht: ----%s\n-------------\n", buff);
-- 
2.9.0