Re: strscpy

Le 04/06/2020 à 01:02, Taylor R Campbell a écrit :
>> Date: Tue, 2 Jun 2020 20:25:45 +0200
>> From: Maxime Villard <[email protected]>
>>
>> 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