Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 23 Apr 2019 14:11:08 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kdb: kdb_io: Replace strcpy() by strscpy()
Message-ID: <20190423131108.mddepqxfy4yraog4@holly.lan>
References: <20190422162711.GA30856@embeddedor>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190422162711.GA30856@embeddedor>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@embeddedor.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
>