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

[Alejandro Colomar <alx@kernel.org>]

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

On Sun, Sep 29, 2024 at 09:58:30AM GMT, Alejandro Colomar wrote:
> [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);

Oops, I misread the original in the above.  I didn't notice that the +=
is being done on the count, not the pointer.  The other equivalences are
good, though.

> 
> ./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.

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

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