Re: strscpy

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 04/06/2020 à 01:02, Taylor R Campbell a écrit :
> > Date: Tue, 2 Jun 2020 20:25:45 +0200
> > From: Maxime Villard <max%m00nbsd.net@localhost>
> >
> > Here is a patch that converts all of the copystr() calls in the kernel to
> > strkcpy(). One exception is vfs_subr.c where it will be done differently
> > and isn't included here.
> >
> > 	https://m00nbsd.net/garbage/libkern/strkcpy.diff
> >
> > This covers the "difficult" cases. The rest is easy and can be switched
> > almost mechanically.
>
> Replacing copystr by your strkcpy strikes me as the opposite of an
> improvement -- it would replace nearly all of the calls to copystr by
> logic that is more complicated, not parallel with the nearby copyinstr
> logic, and harder to audit.

"nearly all": I count five such places. That makes five places in all of
the kernel.

> I would like to see an assessment of how it improves the majority of
> cases, not just how to use it in the handful of difficult edge cases
> you've identified.

Considering that the error checking of strkcpy (or whatever we call it)
is the same as strlcpy's, you can expect to be able to do 'l'->'k' in a
mechanical manner in the vast majority of cases.

As an example, take sys/kern/. The majority of calls to strlcpy do not
check the return value. So you can do 'l'->'k' automatically. Four files
have calls that check the return value: sys_module.c, kern_exec.c,
uipc_domain.c, kern_sysctl.c. In each of these cases, you can do 'l'->'k'
automatically too, because the error checking is the same.

(By the way, it looks like sockaddr_format has some problems, the return
should just be void I think.)

This improves the majority of cases, in that we eliminate the unsafe
strlen behavior of strlcpy, while keeping a code structure that is the
same. Doing the replacement in all of the kernel would quite certainly
fix latent bugs similar to the ones I reported several months ago.

Le 04/06/2020 à 01:40, Robert Elz a écrit :
> ps: all that said, I am no fan of replacing the unbroken copystr() calls
> by anything at all, leave all those alone, just replace strlcpy().

That's something I suggested a few emails ago; we could keep copystr for
only a small selected set of places, and these would just be the five
places where the symmetry with copyinstr is appreciated.

I would want copystr() to be rewritten as a MI libkern C function if we
keep it.

Maxime