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:
signature.asc
Description: PGP signature
