string: Add stracpy and stracpy_pad

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

V2: Use __same_type testing char[], signed char[], and unsigned char[]
Rename to, from, and size, dest, src and count

Quoted 5 lines · Joe Perches

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

Quoted 6 lines

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Sorry, but "compatible types" has a very specific meaning in C, so please don't use that word.

And yes, the kernel disables -Wpointer-sign,
so passing an u8* or s8* when strscpy() expects a char* is silently accepted, but does such code exist?

Quoted 2 lines

V2: Use __same_type testing char[], signed char[], and unsigned char[]
Rename to, from, and size, dest, src and count

count is just as bad as size in terms of "the expression src might contain that identifier". But there's actually no reason to even declare a local variable, just use ARRAY_SIZE() directly as the third argument to strscpy().

Quoted 6 lines · Rasmus Villemoes

Quoted 3 lines · Joe Perches

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Sorry, but "compatible types" has a very specific meaning in C, so please don't use that word.

I think you are being overly pedantic here but what wording do you actually suggest?

Quoted 3 lines

And yes, the kernel disables -Wpointer-sign,
so passing an u8* or s8* when strscpy() expects a char* is silently accepted, but does such code exist?

u8 definitely, s8 I'm not sure.

I don't find via grep a use of s8 foo[] = "bar"; or "signed char foo[] = "bar";

I don't think it bad to allow it.

Quoted 4 lines

count is just as bad as size in terms of "the expression src might contain that identifier". But there's actually no reason to even declare a local variable, just use ARRAY_SIZE() directly as the third argument to strscpy().

I don't care about that myself.
It's a macro local identifier and shadowing in a macro is common. I'm not a big fan of useless underscores.

BUILD_BUG_ON(!__same_type())
strscpy(dst, src, ARRAY_SIZE(dst))

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

A BUILD_BUG is emitted when the type of dest is not compatible.

Quoted 13 lines

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Quoted 2 lines

A BUILD_BUG is emitted when the type of dest is not compatible.

I'm still reluctant to merge this because we don't have code in -next which *uses* it. You did have a patch for that against v1, I believe? Please dust it off and send it along?

Le 25/09/2019 23:50, Andrew Morton a écrit :

Quoted 17 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Quoted 15 lines

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

A BUILD_BUG is emitted when the type of dest is not compatible.

Quoted 3 lines

I'm still reluctant to merge this because we don't have code in -next which *uses* it.  You did have a patch for that against v1, I believe? Please dust it off and send it along?

Joe had a Coccinelle script to mass-convert strlcpy and strscpy. Here's a different patch which converts some of ALSA's strcpy calls to stracpy:

Quoted 18 lines

Le 25/09/2019 23:50, Andrew Morton a écrit :

Quoted 17 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Quoted 15 lines

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

A BUILD_BUG is emitted when the type of dest is not compatible.

Quoted 7 lines

Quoted 3 lines

I'm still reluctant to merge this because we don't have code in -next which *uses* it.  You did have a patch for that against v1, I believe? Please dust it off and send it along?

Joe had a Coccinelle script to mass-convert strlcpy and strscpy. Here's a different patch which converts some of ALSA's strcpy calls to stracpy:

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?

Le 26/09/2019 09:29, Rasmus Villemoes a écrit :

Quoted 10 lines · Stephen Kitt

Quoted 3 lines

Le 25/09/2019 23:50, Andrew Morton a écrit :

Quoted 2 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> 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) &&

#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.

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

Quoted 12 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Quoted 2 lines

A BUILD_BUG is emitted when the type of dest is not compatible.

I'm still reluctant to merge this because we don't have code in -next which *uses* it. You did have a patch for that against v1, I believe? Please dust it off and send it along?

Quoted 14 lines · Andrew Morton

Quoted 12 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Quoted 6 lines

Quoted 2 lines

A BUILD_BUG is emitted when the type of dest is not compatible.

I'm still reluctant to merge this because we don't have code in -next which *uses* it. You did have a patch for that against v1, I believe? Please dust it off and send it along?

https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

I gave up, especially after the snark from Linus where he wrote I don't understand this stuff.

He's just too full of himself here merely using argument from authority.

Creating and using a function like copy_string with both source and destination lengths specified is is also potentially a large source of defects where the stracpy macro atop strscpy does not have a defect path other than the src not being a string at all.

I think the analysis of defects in string function in the kernel is overly difficult today given the number of possible uses of pointer and length in strcpy/strncpy/strlcpy/stracpy.

I think also that there is some sense in what he wrote against the "word salad" use of str<foo>cpy, but using stracpy as a macro when possible instead of strscpy also makes the analysis of defects rather simpler.

The trivial script cocci I posted works well for the simple cases.

https://lore.kernel.org/cocci/[email protected]/

The more complicated cocci script Julia posted is still not quite correct as it required intermediate compilation for verification of specified lengths.

https://lkml.org/lkml/2019/7/25/1406

Tell me again if you still want it and maybe the couple conversions that mm/ would get.

Quoted 14 lines · Andrew Morton

Quoted 12 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Quoted 6 lines

Quoted 2 lines

A BUILD_BUG is emitted when the type of dest is not compatible.

I'm still reluctant to merge this because we don't have code in -next which *uses* it. You did have a patch for that against v1, I believe? Please dust it off and send it along?

https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

I gave up, especially after the snark from Linus where he wrote I don't understand this stuff.

He's just too full of himself here merely using argument from authority.

Creating and using a function like copy_string with both source and destination lengths specified is is also potentially a large source of defects where the stracpy macro atop strscpy does not have a defect path other than the src not being a string at all.

I think the analysis of defects in string function in the kernel is overly difficult today given the number of possible uses of pointer and length in strcpy/strncpy/strlcpy/stracpy.

I think also that there is some sense in what he wrote against the "word salad" use of str<foo>cpy, but using stracpy as a macro when possible instead of strscpy also makes the analysis of defects rather simpler.

The trivial script cocci I posted works well for the simple cases.

https://lore.kernel.org/cocci/[email protected]/

The more complicated cocci script Julia posted is still not quite correct as it required intermediate compilation for verification of specified lengths.

https://lkml.org/lkml/2019/7/25/1406

Tell me again if you still want it and maybe the couple conversions that mm/ would get.

via:

$ spatch --all-includes --in-place -sp-file str.cpy.cocci mm
$ git diff --stat -p mm
--
 mm/dmapool.c | 2 +-
 mm/zswap.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

mm/dmapool.c +1 -1

index fe5d33060415..b3a4feb423f8 100644
@@ -158,7 +158,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	if (!retval)
 		return retval;
 
-	strlcpy(retval->name, name, sizeof(retval->name));
+	stracpy(retval->name, name);
 
 	retval->dev = dev;
 

mm/zswap.c +1 -1

index 08b6cefae5d8..c6cd38de185a 100644
@@ -533,7 +533,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	}
 	pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
 
-	strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
+	stracpy(pool->tfm_name, compressor);
 	pool->tfm = alloc_percpu(struct crypto_comp *);
 	if (!pool->tfm) {
 		pr_err("percpu alloc failed\n");

Quoted 14 lines · Andrew Morton

Quoted 12 lines · On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <[email protected]> wrote:

Several uses of strlcpy and strscpy have had defects because the last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string array as the first argument (dest) and uses sizeof(dest) as the count of bytes to copy.

These mechanisms verify that the dest argument is an array of char or other compatible types like u8 or s8 or equivalent.

Quoted 6 lines

Quoted 2 lines

A BUILD_BUG is emitted when the type of dest is not compatible.

I'm still reluctant to merge this because we don't have code in -next which *uses* it. You did have a patch for that against v1, I believe? Please dust it off and send it along?

https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

I gave up, especially after the snark from Linus where he wrote I don't understand this stuff.

He's just too full of himself here merely using argument from authority.

Creating and using a function like copy_string with both source and destination lengths specified is is also potentially a large source of defects where the stracpy macro atop strscpy does not have a defect path other than the src not being a string at all.

I think the analysis of defects in string function in the kernel is overly difficult today given the number of possible uses of pointer and length in strcpy/strncpy/strlcpy/stracpy.

I think also that there is some sense in what he wrote against the "word salad" use of str<foo>cpy, but using stracpy as a macro when possible instead of strscpy also makes the analysis of defects rather simpler.

The trivial script cocci I posted works well for the simple cases.

https://lore.kernel.org/cocci/[email protected]/

The more complicated cocci script Julia posted is still not quite correct as it required intermediate compilation for verification of specified lengths.

https://lkml.org/lkml/2019/7/25/1406

Tell me again if you still want it and maybe the couple conversions that mm/ would get.

via:

$ spatch --all-includes --in-place -sp-file str.cpy.cocci mm
$ git diff --stat -p mm
--
 mm/dmapool.c | 2 +-
 mm/zswap.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

mm/dmapool.c +1 -1

index fe5d33060415..b3a4feb423f8 100644
@@ -158,7 +158,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	if (!retval)
 		return retval;
 
-	strlcpy(retval->name, name, sizeof(retval->name));
+	stracpy(retval->name, name);
 
 	retval->dev = dev;
 

mm/zswap.c +1 -1

index 08b6cefae5d8..c6cd38de185a 100644
@@ -533,7 +533,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	}
 	pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
 
-	strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
+	stracpy(pool->tfm_name, compressor);
 	pool->tfm = alloc_percpu(struct crypto_comp *);
 	if (!pool->tfm) {
 		pr_err("percpu alloc failed\n");