Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

[CC += Andy, Gustavo]

On Sat, Sep 28, 2024 at 02:17:30PM GMT, Kees Cook wrote:
> > > diff --git a/mm/util.c b/mm/util.c
> > > index 983baf2bd675..4542d8a800d9 100644
> > > --- a/mm/util.c
> > > +++ b/mm/util.c
> > > @@ -62,8 +62,14 @@ char *kstrdup(const char *s, gfp_t gfp)
> > >  
> > >  	len = strlen(s) + 1;
> > >  	buf = kmalloc_track_caller(len, gfp);
> > > -	if (buf)
> > > +	if (buf) {
> > >  		memcpy(buf, s, len);
> > > +		/* During memcpy(), the string might be updated to a new value,
> > > +		 * which could be longer than the string when strlen() is
> > > +		 * called. Therefore, we need to add a null termimator.
> > > +		 */
> > > +		buf[len - 1] = '\0';
> > > +	}
> > 
> > I would compact the above to:
> > 
> > 	len = strlen(s);
> > 	buf = kmalloc_track_caller(len + 1, gfp);
> > 	if (buf)
> > 		strcpy(mempcpy(buf, s, len), "");
> > 
> > It allows _FORTIFY_SOURCE to track the copy of the NUL, and also uses
> > less screen.  It also has less moving parts.  (You'd need to write a
> > mempcpy() for the kernel, but that's as easy as the following:)
> > 
> > 	#define mempcpy(d, s, n)  (memcpy(d, s, n) + n)
> > 
> > In shadow utils, I did a global replacement of all buf[...] = '\0'; by
> > strcpy(..., "");.  It ends up being optimized by the compiler to the
> > same code (at least in the experiments I did).
> 
> Just to repeat what's already been said: no, please, don't complicate
> this with yet more wrappers. And I really don't want to add more str/mem
> variants -- we're working really hard to _remove_ them. :P

Hi Kees,

I assume by "[no] more str/mem variants" you're referring to mempcpy(3).

mempcpy(3) is a libc function available in several systems (at least
glibc, musl, FreeBSD, and NetBSD).  It's not in POSIX nor in OpenBSD,
but it's relatively widely available.  Availability is probably
pointless to the kernel, but I mention it because it's not something
random I came up with, but rather something that several projects have
found useful.  I find it quite useful to copy the non-zero part of a
string.  See string_copying(7).
<https://www.man7.org/linux/man-pages/man7/string_copying.7.html>

Regarding "we're working really hard to remove them [mem/str wrappers]",
I think it's more like removing those that are prone to misuse, not just
blinly reducing the amount of wrappers.  Some of them are really useful.

I've done a randomized search of kernel code, and found several places
where mempcpy(3) would be useful for simplifying code:

./drivers/staging/rtl8723bs/core/rtw_ap.c:		memcpy(pwps_ie, pwps_ie_src, wps_ielen + 2);
./drivers/staging/rtl8723bs/core/rtw_ap.c-		pwps_ie += (wps_ielen+2);

equivalent to:

	pwps_ie = mempcpy(pwps_ie, pwps_ie_src, wps_ielen + 2);

./drivers/staging/rtl8723bs/core/rtw_ap.c:		memcpy(supportRate + supportRateNum, p + 2, ie_len);
./drivers/staging/rtl8723bs/core/rtw_ap.c-		supportRateNum += ie_len;

equivalent to:

	supportRateNum = mempcpy(supportRate + supportRateNum, p + 2, ie_len);

./drivers/staging/rtl8723bs/core/rtw_ap.c:		memcpy(dst_ie, &tim_bitmap_le, 2);
./drivers/staging/rtl8723bs/core/rtw_ap.c-		dst_ie += 2;

equivalent to:

	dst_ie = mempcpy(dst_ie, &tim_bitmap_le, 2);


And there are many cases like this.  Using mempcpy(3) would make this
pattern less repetitive.


Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]