linux-kernel - Re: [PATCH] kdb: kdb_io: Replace strcpy() by strscpy()

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

Date:   Tue, 23 Apr 2019 14:11:08 +0100
From:   Daniel Thompson <daniel.thompson@...aro.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kdb: kdb_io: Replace strcpy() by strscpy()

On Mon, Apr 22, 2019 at 11:27:11AM -0500, Gustavo A. R. Silva wrote:
> The strcpy() function is being deprecated. Replace it by the safer
> strscpy() and fix the following Coverity warning:
> 
> "You might overrun the 256-character fixed-size string kdb_buffer
> by copying cphold without checking the length."
> 
> Addresses-Coverity-ID: 138996 ("Copy into fixed size buffer")
> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.com>
> ---
>  kernel/debug/kdb/kdb_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 6a4b41484afe..ebc4aa2d0737 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -836,7 +836,7 @@ int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
>  	 */
>  	if (kdb_grepping_flag && !suspend_grep) {
>  		*cphold = replaced_byte;
> -		strcpy(kdb_buffer, cphold);
> +		strscpy(kdb_buffer, cphold, sizeof(kdb_buffer));

This looks like a mechanical or semi-mechanical fix... I think it
misses a couple of things.

Firstly this code pattern appears twice in the file but you have only
fixed on of those instances. If this fix is required then both should
be changed.

Secondly cphold is a pointer into kdb_buffer itself which means that
*neither* strcpy() nor strscpy() are safe (since their behaviour is
undefined) so we should probably be using memmove() anyway.

Daniel.

PS there is an range change since a sub-string is always shorted than a
   string (albeit an implicit one based on correct termination handling)
   so a strlen()+memmove() fix is probably OK here.

Daniel.

>  		len = strlen(kdb_buffer);
>  		next_avail = kdb_buffer + len;
>  		size_avail = sizeof(kdb_buffer) - len;
> -- 
> 2.21.0
>