linux-kernel - Re: [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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d9fb37d-7dd7-596d-42dd-bd0d6580bd43@gmail.com>
Date:   Tue, 13 Mar 2018 10:04:29 -0600
From:   Martin Sebor <msebor@...il.com>
To:     Arnd Bergmann <arnd@...db.de>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Paul Blakey <paulb@...lanox.com>
Cc:     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: Re: [PATCH] test_rhashtable: avoid gcc-8 -Wformat-overflow warning

On 03/13/2018 07:21 AM, Arnd Bergmann wrote:
> 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.

A couple of other approaches that may be worth considering are
to either a) use a precision in the %s directive to constrain
the number of characters to copy (e.g., %.490s") or b) call
snprintf() instead and use the returned value to handle
the possible truncation.

For GCC 9 we'd like to integrate the warning with the strlen pass
that tracks string lengths.  That should add another mechanism for
suppressing the warning: add an assertion before the sprintf call
bounding the string length.

Martin

> 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);
>