Removing more str APIs (was Re: [PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style)

[Kees Cook]

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@xxxxxxxxxxxx> 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@xxxxxxxxxx/

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