Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 26/09/2019 10.25, Stephen Kitt wrote:
> Le 26/09/2019 09:29, Rasmus Villemoes a ÃcritÂ:
>> On 26/09/2019 02.01, Stephen Kitt wrote:
>>> Le 25/09/2019 23:50, Andrew Morton a ÃcritÂ:
>>>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@xxxxxxxxxxx> wrote:
>>>>
>>
>> Please don't. At least not for the cases where the source is a string
>> literal - that just gives worse code generation (because gcc doesn't
>> know anything about strscpy or strlcpy), and while a run-time (silent)
>> truncation is better than a run-time buffer overflow, wouldn't it be
>> even better with a build time error?
> 
> Yes, that was the plan once Joe's patch gets merged (if it does), and my
> patch was only an example of using stracpy, as a step on the road. I was
> intending to follow up with a patch converting stracpy to something like
> https://www.openwall.com/lists/kernel-hardening/2019/07/06/14
> 
> __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> {
> ÂÂÂÂsize_t dest_size = __builtin_object_size(dest, 0);
> ÂÂÂÂsize_t src_size = __builtin_object_size(src, 0);
> ÂÂÂÂif (__builtin_constant_p(count) &&
> ÂÂÂÂÂÂÂ __builtin_constant_p(src_size) &&
> ÂÂÂÂÂÂÂ __builtin_constant_p(dest_size) &&

Eh? Isn't the output of __builtin_object_size() by definition a
compile-time constant - whatever the compiler happens to know about the
object size (or a sentinel 0 or -1 depending on the type argument)?

> 
> #define stracpy(dest, src)ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ({ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂsize_t count = ARRAY_SIZE(dest);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂsize_t dest_size = __builtin_object_size(dest, 0);ÂÂÂÂÂÂÂ \
> ÂÂÂÂsize_t src_size = __builtin_object_size(src, 0);ÂÂÂÂÂÂÂ \
> ÂÂÂÂBUILD_BUG_ON(!(__same_type(dest, char[]) ||ÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __same_type(dest, unsigned char[]) ||ÂÂÂÂÂÂÂ \
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __same_type(dest, signed char[])));ÂÂÂÂÂÂÂ \
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ(__builtin_constant_p(count) &&ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ __builtin_constant_p(src_size) &&ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ __builtin_constant_p(dest_size) &&ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ src_size <= count &&ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ src_size <= dest_size &&ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂ src[src_size - 1] == '\0') ?ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂÂÂÂ (((size_t) strcpy(dest, src)) & 0) + src_size - 1ÂÂÂ \
> ÂÂÂÂ:ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> ÂÂÂÂÂÂÂ strscpy(dest, src, count);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> })
> 
> and both of these get optimised to movs when copying a constant string
> which fits in the target.

But does it catch the case of overflowing a char[] member in a struct
passed by reference at build time? I'm surprised that
__builtin_object_size(dest, 0) seems to be (size_t)-1, when dest is
s->name (with struct s { char name[4]; };). So I'm not very confident
that any of the fancy fortify logic actually works here - we _really_
should have some Kbuild infrastructure for saying "this .c file should
not compile" so we can test that the fortifications actually work in the
simple and common cases.

> I was going at this from the angle of improving the existing APIs and
> their resulting code.

I'm not against stracpy() as a wrapper for strscpy(), but we should make
sure that whenever we can fail at build time (i.e., both source and dst
lengths known), we do - and in that case also let the compiler optimize
the copy (not only to do the immediate movs, but that also gives it
wider opportunity to remove it completely as dead stores if the
surrounding code ends up dead - with a call to some strscpy(), gcc
cannot eliminate that). If stracpy() can be made sufficiently magic that
it fails at build time for the string literal cases, fine, let's not add
yet another API. Otherwise, I think the static_strcpy() is a much more
readable and reliable API for those cases.

If I'm reading your stracpy() macro correctly, you're explicitly
requesting a run-time truncation (the src_size <= dest_size check
causing as to fall back to strscpy) rather than failing at build time.

Rasmus