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@perches.com 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