linux-kernel - Re: [PATCH v3] kdb: Replace deprecated strcpy() with strscpy() and memcpy()

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: <aJ9EPQNk2bcOMP1h@aspen.lan>
Date: Fri, 15 Aug 2025 15:29:17 +0100
From: Daniel Thompson <daniel@...cstar.com>
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>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	linux-hardening@...r.kernel.org,
	kgdb-bugreport@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] kdb: Replace deprecated strcpy() with strscpy() and
 memcpy()

On Fri, Aug 15, 2025 at 03:16:27PM +0200, Thorsten Blum wrote:
> On 15. Aug 2025, at 13:40, Daniel Thompson wrote:
> > On Fri, Aug 15, 2025 at 01:28:01PM +0200, Thorsten Blum wrote:
> >> Hi Daniel,
> >>
> >>> On 15. Aug 2025, at 10:57, Daniel Thompson wrote:
> >>> Sorry but a strscpy() where the length of the destination buffer has
> >>> been calculated from the source string is way too much of a red flag
> >>> for me.
> >>>
> >>> Put another way if there are "no functional changes intended" then there
> >>> cannot possibly be any security benefit from replacing the "unsafe"
> >>> strcpy() with the "safe" strscpy(). Likewise abusing the destination
> >>> length argument to truncate a string makes the code shorter but *not*
> >>> clearer because it's too easy to misread.
> >>
> >> Deliberately truncating the source using strscpy() is a valid use case.
> >> strscpy() allows the size argument to be smaller than the destination
> >> buffer, so this is an intended use of the size argument, not an abuse.
> >
> > Sorry, I didn't phrase that especially well. I regard the abuse to be
> > deriving the length of the destination buffer exclusively from the
> > state of the source buffer.
> >
> > As mentioned, it would be much cleaner to eliminate the string copy entirely
> > than to translate it into something so similar to the original strcpy().
>
> Something like this?

It would feels disingenuous of me say "exactly like this" because I
think your code is nicer than mine would have been for this (I suspect
I would have been lazy and kdb_strdup()'ed str+1 and injected a
terminator)!

Looks great. Only one comment:

> char *kdb_strdup_dequote(const char *str, gfp_t type)
> {
> 	size_t len = strlen(str);
> 	char *s;
>
> 	if (str[0] == '"') {
> 		/* skip leading quote */
> 		len--;
> 		str++;
>
> 		if (len > 0 && str[len - 1] == '"')
> 			len--; /* skip trailing quote */

I like the extra diligence of checking the trailing quote but perhaps
combine the two if statements (so we only chomp the quotes if there
are two).


>        }
>
> 	len++; /* add space for NUL terminator */
>
> 	s = kmalloc(len, type);
> 	if (!s)
> 		return NULL;
> 	strscpy(s, str, len);
> 	return s;
> }
>
> This should probably be a separate patch, right?

I think so.

I generally figure if you have to put two paragraphs into the patch
description but each paragraph makes sense in isolation that's a sign
there should be two patches... and I think that would be the case here
(the paragraph explaining the memcpy() piece and the paragraph
explaining kdb_strdup_dequote() would make sense in isolation.


Daniel.