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

[Daniel Thompson <daniel@riscstar.com>]

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.