linux-kernel - Re: [PATCH] kdb: Replace deprecated strcpy() with 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025081456-reflected-revolver-9f5b@gregkh>
Date: Thu, 14 Aug 2025 16:35:48 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Jason Wessel <jason.wessel@...driver.com>,
	Daniel Thompson <danielt@...nel.org>,
	Douglas Anderson <dianders@...omium.org>,
	Nir Lichtman <nir@...htman.org>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	linux-hardening@...r.kernel.org,
	Daniel Thompson <daniel@...cstar.com>,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kdb: Replace deprecated strcpy() with strscpy()

On Thu, Aug 14, 2025 at 02:35:56PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 14, 2025 at 02:03:37PM +0200, Thorsten Blum wrote:
> > strcpy() is deprecated; use strscpy() instead and remove several manual
> > NUL-terminations.
> 
> Manual NULL terminations are good, why get rid of that?
> 
> > Since the destination buffers 'cmd_cur' and 'cmd_hist[cmd_head]' have
> > the fixed length CMD_BUFLEN, strscpy() automatically determines their
> > size using sizeof() when the size argument is omitted. This makes the
> > explicit size arguments for the existing strscpy() calls unnecessary,
> > remove them.
> 
> But now you are dynamically calculating this?
> 
> > No functional changes intended.
> 
> How did you test this?  Many of these types of changes are wrong, so you
> really really need to prove it is correct.
> 
> > 
> > Link: https://github.com/KSPP/linux/issues/88
> > Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> > ---
> >  kernel/debug/kdb/kdb_main.c | 32 ++++++++++++++------------------
> >  1 file changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 7a4d2d4689a5..ea7dc2540e40 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -727,14 +727,10 @@ static int kdb_defcmd(int argc, const char **argv)
> >  	mp->help = kdb_strdup(argv[3], GFP_KDB);
> >  	if (!mp->help)
> >  		goto fail_help;
> > -	if (mp->usage[0] == '"') {
> > -		strcpy(mp->usage, argv[2]+1);
> > -		mp->usage[strlen(mp->usage)-1] = '\0';
> > -	}
> > -	if (mp->help[0] == '"') {
> > -		strcpy(mp->help, argv[3]+1);
> > -		mp->help[strlen(mp->help)-1] = '\0';
> > -	}
> > +	if (mp->usage[0] == '"')
> > +		strscpy(mp->usage, argv[2] + 1, strlen(argv[2]) - 1);
> 
> Now you are manually testing the length of argv[2], are you sure that's
> ok?
> 
> > +	if (mp->help[0] == '"')
> > +		strscpy(mp->help, argv[3] + 1, strlen(argv[3]) - 1);
> >  
> >  	INIT_LIST_HEAD(&kdb_macro->statements);
> >  	defcmd_in_progress = true;
> > @@ -860,7 +856,7 @@ static void parse_grep(const char *str)
> >  		kdb_printf("search string too long\n");
> >  		return;
> >  	}
> > -	strcpy(kdb_grep_string, cp);
> > +	strscpy(kdb_grep_string, cp);
> 
> If this was just a search/replace, it would have been done already, so
> why is this ok?

I missed that strscpy() can now handle 2 arguments like this, so yes,
this should be ok.

BUT, you just checked the length above this line, which now isn't
needed, right?  So this function can get simpler?


> 
> 
> >  	kdb_grepping_flag++;
> >  	return;
> >  }
> > @@ -1076,12 +1072,12 @@ static int handle_ctrl_cmd(char *cmd)
> >  		if (cmdptr != cmd_tail)
> >  			cmdptr = (cmdptr + KDB_CMD_HISTORY_COUNT - 1) %
> >  				 KDB_CMD_HISTORY_COUNT;
> > -		strscpy(cmd_cur, cmd_hist[cmdptr], CMD_BUFLEN);
> > +		strscpy(cmd_cur, cmd_hist[cmdptr]);
> 
> Same here.  And other places...

Sorry, this should also be ok, BUT it's really just doing the same exact
thing, right?  And, it's a different thing, so it should be a different
patch (i.e. do not mix different logical things in the same patch, it
confuses everyone.  Well, me at least...)

thanks,

greg k-h