Messages in this thread | | | Date | Wed, 7 Feb 2024 02:42:42 -0800 | From | Kees Cook <> | Subject | Removing more str APIs (was Re: [PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style) |
| |
On Tue, Feb 06, 2024 at 05:02:16PM +0200, Andy Shevchenko wrote: > On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@chromium.org> wrote: > > > > The ARCH=um build has its own idea about strscpy()'s definition. Adjust > > the callers to remove the redundant sizeof() arguments ahead of treewide > > changes, since it needs a manual adjustment for the newly named > > sized_strscpy() export. > > ... > > > - strscpy(dir, home, sizeof(dir)); > > + strscpy(dir, home); > > uml_dir++; > > } > > strlcat(dir, uml_dir, sizeof(dir)); > > An (unrelated) side note: are we going to get rid of strlcat() as well > (after strlcpy() is gone)?
I think it would be worthwhile to remove it, yes. Switching to seq_buf in many cases seemed to be the clear solution, which is what triggered my trying to improve the allocation ergonomics for seq_buf recently: https://lore.kernel.org/linux-hardening/20231027155634.make.260-kees@kernel.org/
Its not in super common usage, so I think we can start chipping away at it:
$ git grep '\bstrlcat(' | wc -l 480
It's more risky cases (using the return value) is relatively rare, though, so I hadn't been prioritizing its removal:
$ git grep ' = strlcat(\b' | wc -l 13
(And almost all of it is in security/selinux/ima.c)
As a comparison, strncpy has even fewer currently, with Justin making a dent on it recently:
$ git grep '\bstrncpy(' | wc -l 311
> > ... > > > if (*umid == '\0') { > > - strscpy(tmp, uml_dir, sizeof(tmp)); > > + strscpy(tmp, uml_dir); > > strlcat(tmp, "XXXXXX", sizeof(tmp)); > > This code is interesting... (Esp. taking into account making a > temporary folder out of this...)
I have tried to avoid reading UML code too closely. ;)
-- Kees Cook
| |