Fix GCC -Wstringop-truncation warnings

PATCH In review Stafford Horne 3 participants 9 messages Jun 25, 2018 – Jul 8, 2018

Subsystems (3)

Library code Crypto API Driver core

lore

Share & tools

lore

Stafford Horne

author

2018-06-25 12:45 UTC #0 lore raw

v2 cover Fix GCC -Wstringop-truncation warnings

Hello,

When compiling OpenRISC kernels with our new toolchain based on GCC 9.0.0 I am seeing various -Wstringop-truncation warnings. There might be more as I am not compiling all drivers/modules yet, if someone thinks thats helpful let me know.

I discussed this with Greg KH at the OSS Summit Japan 2018 and it seems no one has pointed these out yet, so here are the patches... Actually, the crypto issue was reported before, but the patch was discarded as it introduced a data leakage bug pointed out by Erix.

As for merging, I think the maintainers should pick these up separately. Let me know if you want something else.

Changes since v1:

- Fix paper-bag bug in kobject patch, using memcpy() now
- Fix data leakage issue crypto patch pointed out by Eric Biggers

-Stafford

Stafford Horne (2):
crypto: Fix -Wstringop-truncation warnings
kobject: Fix -Wstringop-truncation warning

 crypto/ablkcipher.c | 2 ++
 crypto/blkcipher.c  | 1 +
 lib/kobject.c       | 2 +-
 3 files changed, 4 insertions(+), 1 deletion(-)

Signature · 1 line

2.17.0

Stafford Horne

author

2018-06-25 12:45 UTC #1 lore raw

PATCH v2 1/2 crypto: Fix -Wstringop-truncation warnings

As of GCC 9.0.0 the build is reporting warnings like:

    crypto/ablkcipher.c: In function ‘crypto_ablkcipher_report’:
    crypto/ablkcipher.c:374:2: warning: ‘strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
      strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       sizeof(rblkcipher.geniv));
       ~~~~~~~~~~~~~~~~~~~~~~~~~

This means the strnycpy might create a non null terminated string. Fix this by explicitly performing '\0' termination.

Cc Greg Kroah-Hartman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Max Filippov <[email protected]>

Cc Eric Biggers <[email protected]>

Cc Nick Desaulniers <[email protected]>

Signed-off-by Stafford Horne <[email protected]>

---
 crypto/ablkcipher.c | 2 ++
 crypto/blkcipher.c  | 1 +
 2 files changed, 3 insertions(+)

crypto/ablkcipher.c +2 -0

index d880a4897159..1edb5000d783 100644
@@ -373,6 +373,7 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
 	strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
 	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
 		sizeof(rblkcipher.geniv));
+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
 	rblkcipher.blocksize = alg->cra_blocksize;
 	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

@@ -447,6 +448,7 @@ static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
 	strncpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type));
 	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<built-in>",
 		sizeof(rblkcipher.geniv));
+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
 	rblkcipher.blocksize = alg->cra_blocksize;
 	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

crypto/blkcipher.c +1 -0

index 01c0d4aa2563..dd4dcab3766a 100644
@@ -512,6 +512,7 @@ static int crypto_blkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
 	strncpy(rblkcipher.type, "blkcipher", sizeof(rblkcipher.type));
 	strncpy(rblkcipher.geniv, alg->cra_blkcipher.geniv ?: "<default>",
 		sizeof(rblkcipher.geniv));
+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
 	rblkcipher.blocksize = alg->cra_blocksize;
 	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;

Signature · 1 line

2.17.0

Christophe LEROY

2018-06-25 12:59 UTC #2 lore raw

Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings

Le 25/06/2018 à 14:45, Stafford Horne a écrit :

Quoted 28 lines

As of GCC 9.0.0 the build is reporting warnings like:
     crypto/ablkcipher.c: In function ‘crypto_ablkcipher_report’:
     crypto/ablkcipher.c:374:2: warning: ‘strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
       strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        sizeof(rblkcipher.geniv));
        ~~~~~~~~~~~~~~~~~~~~~~~~~
This means the strnycpy might create a non null terminated string. Fix this by explicitly performing '\0' termination.
Cc Greg Kroah-Hartman <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Max Filippov <[email protected]>
Cc Eric Biggers <[email protected]>
Cc Nick Desaulniers <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
  crypto/ablkcipher.c | 2 ++
  crypto/blkcipher.c  | 1 +
  2 files changed, 3 insertions(+)
crypto/ablkcipher.c +0 -0
index d880a4897159..1edb5000d783 100644

crypto/ablkcipher.c static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) · line 375

Quoted 3 lines

crypto/ablkcipher.c +0 -0

@@ -373,6 +373,7 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
  		sizeof(rblkcipher.geniv));

Is it worth copying something we are going to discard at the following line ? Shouldn't you limit the copy to sizeof(rblkcipher.geniv) - 1 ?

crypto/ablkcipher.c static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) · line 450

Quoted 8 lines

crypto/ablkcipher.c +1 -0

+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

@@ -447,6 +448,7 @@ static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<built-in>",
  		sizeof(rblkcipher.geniv));

Same comment here.

Christophe

Quoted 17 lines

crypto/ablkcipher.c +1 -0

+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

crypto/blkcipher.c +1 -0

index 01c0d4aa2563..dd4dcab3766a 100644
@@ -512,6 +512,7 @@ static int crypto_blkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "blkcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_blkcipher.geniv ?: "<default>",
  		sizeof(rblkcipher.geniv));
+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
 
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;

Stafford Horne

author

2018-06-25 13:40 UTC #3 lore raw

Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings

Quoted 26 lines · Christophe LEROY

Le 25/06/2018 à 14:45, Stafford Horne a écrit :
Quoted 23 lines
As of GCC 9.0.0 the build is reporting warnings like:
     crypto/ablkcipher.c: In function ‘crypto_ablkcipher_report’:
     crypto/ablkcipher.c:374:2: warning: ‘strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
       strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        sizeof(rblkcipher.geniv));
        ~~~~~~~~~~~~~~~~~~~~~~~~~
This means the strnycpy might create a non null terminated string. Fix this by explicitly performing '\0' termination.
Cc Greg Kroah-Hartman <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Max Filippov <[email protected]>
Cc Eric Biggers <[email protected]>
Cc Nick Desaulniers <[email protected]>
Signed-off-by Stafford Horne <[email protected]>
---
  crypto/ablkcipher.c | 2 ++
  crypto/blkcipher.c  | 1 +
  2 files changed, 3 insertions(+)

Quoted 11 lines

crypto/ablkcipher.c static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg) · line 375
Quoted 8 lines
crypto/ablkcipher.c +0 -0
index d880a4897159..1edb5000d783 100644
@@ -373,6 +373,7 @@ static int crypto_ablkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "ablkcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
  		sizeof(rblkcipher.geniv));
Is it worth copying something we are going to discard at the following line
? Shouldn't you limit the copy to sizeof(rblkcipher.geniv) - 1 ?

Hi,

I thought about that, I just did it like this as I thought it might be easier to read and I noticed a few other areas in the kernel that did this way. After a closer look I can see we have both patterns, perhaps we need a mcro/helper.

I don't mind either way, I can fix, if the crypto maintainers want to adjust the patch that would work too.

-Stafford

Quoted 27 lines

crypto/ablkcipher.c static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg) · line 450

Quoted 7 lines

crypto/ablkcipher.c +1 -0

+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

@@ -447,6 +448,7 @@ static int crypto_givcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "givcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<built-in>",
  		sizeof(rblkcipher.geniv));

Same comment here.

Christophe

Quoted 15 lines

crypto/ablkcipher.c +1 -0

+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;

crypto/blkcipher.c +1 -0

index 01c0d4aa2563..dd4dcab3766a 100644
@@ -512,6 +512,7 @@ static int crypto_blkcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
  	strncpy(rblkcipher.type, "blkcipher", sizeof(rblkcipher.type));
  	strncpy(rblkcipher.geniv, alg->cra_blkcipher.geniv ?: "<default>",
  		sizeof(rblkcipher.geniv));
+	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
  	rblkcipher.blocksize = alg->cra_blocksize;
  	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;

Herbert Xu

maintainer

2018-07-08 16:43 UTC #4 lore raw

Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings

In reply to Stafford Horne

Quoted 12 lines · Stafford Horne

As of GCC 9.0.0 the build is reporting warnings like:

    crypto/ablkcipher.c: In function ‘crypto_ablkcipher_report’:
    crypto/ablkcipher.c:374:2: warning: ‘strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
      strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       sizeof(rblkcipher.geniv));
       ~~~~~~~~~~~~~~~~~~~~~~~~~

This means the strnycpy might create a non null terminated string. Fix this by explicitly performing '\0' termination.

Quoted 6 lines

Cc Greg Kroah-Hartman <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Max Filippov <[email protected]>
Cc Eric Biggers <[email protected]>
Cc Nick Desaulniers <[email protected]>
Signed-off-by Stafford Horne <[email protected]>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Stafford Horne

author

2018-06-25 12:45 UTC #5 lore raw

PATCH v2 2/2 kobject: Fix -Wstringop-truncation warning

In reply to Stafford Horne

When compiling with GCC 9.0.0 I am seeing the following warning:

In function ‘fill_kobj_path’,
inlined from ‘kobject_get_path’ at lib/kobject.c:155:2:
lib/kobject.c:128:3: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
strncpy(path + length, kobject_name(parent), cur);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/kobject.c: In function ‘kobject_get_path’:
lib/kobject.c:125:13: note: length computed here
int cur = strlen(kobject_name(parent));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is not really an issue since the buffer we are writing to is pre-zero'd and we have already allocated the buffer based on the calculated strlen size and accounted for the terminating '\0'. Just use memcpy() instead.

Cc Greg Kroah-Hartman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Eric Biggers <[email protected]>

Signed-off-by Stafford Horne <[email protected]>

---
 lib/kobject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/kobject.c +1 -1

index 18989b5b3b56..e876957743c8 100644
@@ -125,7 +125,7 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
 		int cur = strlen(kobject_name(parent));
 		/* back up enough to print this name with '/' */
 		length -= cur;
-		strncpy(path + length, kobject_name(parent), cur);
+		memcpy(path + length, kobject_name(parent), cur);
 		*(path + --length) = '/';
 	}

Signature · 1 line

2.17.0

Christophe LEROY

2018-06-25 12:57 UTC #6 lore raw

Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning

Le 25/06/2018 à 14:45, Stafford Horne a écrit :

Quoted 12 lines

When compiling with GCC 9.0.0 I am seeing the following warning:
In function ‘fill_kobj_path’,
inlined from ‘kobject_get_path’ at lib/kobject.c:155:2:
lib/kobject.c:128:3: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
strncpy(path + length, kobject_name(parent), cur);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/kobject.c: In function ‘kobject_get_path’:
lib/kobject.c:125:13: note: length computed here
int cur = strlen(kobject_name(parent));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 4 lines

This is not really an issue since the buffer we are writing to is pre-zero'd and we have already allocated the buffer based on the calculated strlen size and accounted for the terminating '\0'. Just use memcpy() instead.

If we are already sure the destination is big enough, why not just do a strcpy() and drop the 'cur = strlen()' ?

Christophe

Quoted 23 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Eric Biggers <[email protected]>

Signed-off-by Stafford Horne <[email protected]>

---
  lib/kobject.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

lib/kobject.c +1 -1

index 18989b5b3b56..e876957743c8 100644
@@ -125,7 +125,7 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
  		int cur = strlen(kobject_name(parent));
  		/* back up enough to print this name with '/' */
  		length -= cur;
-		strncpy(path + length, kobject_name(parent), cur);
+		memcpy(path + length, kobject_name(parent), cur);
  		*(path + --length) = '/';
  	}

Stafford Horne

author

2018-06-25 13:24 UTC #7 lore raw

Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning

Quoted 15 lines · Christophe LEROY

Le 25/06/2018 à 14:45, Stafford Horne a écrit :
Quoted 12 lines
When compiling with GCC 9.0.0 I am seeing the following warning:
In function ‘fill_kobj_path’,
inlined from ‘kobject_get_path’ at lib/kobject.c:155:2:
lib/kobject.c:128:3: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
strncpy(path + length, kobject_name(parent), cur);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/kobject.c: In function ‘kobject_get_path’:
lib/kobject.c:125:13: note: length computed here
int cur = strlen(kobject_name(parent));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Quoted 7 lines

Quoted 4 lines
This is not really an issue since the buffer we are writing to is pre-zero'd and we have already allocated the buffer based on the calculated strlen size and accounted for the terminating '\0'. Just use memcpy() instead.
If we are already sure the destination is big enough, why not just do a strcpy() and drop the 'cur = strlen()' ?

Hi Christophe,

Here were are writing multiple strings into a buffer from back to front. We are copying exactly strlen() bytes at a time to avoid the nul terminator being copied into the buffer.

I don't doubt we could use strcpy() but I was trying to keep the change small.

-Stafford

Quoted 22 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Eric Biggers <[email protected]>

Signed-off-by Stafford Horne <[email protected]>

---
  lib/kobject.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

lib/kobject.c +1 -1

index 18989b5b3b56..e876957743c8 100644
@@ -125,7 +125,7 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
  		int cur = strlen(kobject_name(parent));
  		/* back up enough to print this name with '/' */
  		length -= cur;
-		strncpy(path + length, kobject_name(parent), cur);
+		memcpy(path + length, kobject_name(parent), cur);
  		*(path + --length) = '/';
  	}

Christophe LEROY

Reviewed-by

2018-06-25 13:32 UTC #8 lore raw

Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning

Le 25/06/2018 à 15:24, Stafford Horne a écrit :

Quoted 24 lines · Christophe LEROY

Quoted 15 lines
Le 25/06/2018 à 14:45, Stafford Horne a écrit :
Quoted 12 lines
When compiling with GCC 9.0.0 I am seeing the following warning:
In function ‘fill_kobj_path’,
inlined from ‘kobject_get_path’ at lib/kobject.c:155:2:
lib/kobject.c:128:3: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
strncpy(path + length, kobject_name(parent), cur);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/kobject.c: In function ‘kobject_get_path’:
lib/kobject.c:125:13: note: length computed here
int cur = strlen(kobject_name(parent));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Quoted 7 lines
Quoted 4 lines
This is not really an issue since the buffer we are writing to is pre-zero'd and we have already allocated the buffer based on the calculated strlen size and accounted for the terminating '\0'. Just use memcpy() instead.
If we are already sure the destination is big enough, why not just do a strcpy() and drop the 'cur = strlen()' ?

Quoted 7 lines

Hi Christophe,
Here were are writing multiple strings into a buffer from back to front. We are copying exactly strlen() bytes at a time to avoid the nul terminator being copied into the buffer.
I don't doubt we could use strcpy() but I was trying to keep the change small.

Ok, fair enough.

Reviewed-by Christophe Leroy <[email protected]>

Quoted 25 lines

-Stafford

Quoted 22 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Eric Biggers <[email protected]>

Signed-off-by Stafford Horne <[email protected]>

---
   lib/kobject.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

lib/kobject.c +1 -1

index 18989b5b3b56..e876957743c8 100644
@@ -125,7 +125,7 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
   		int cur = strlen(kobject_name(parent));
   		/* back up enough to print this name with '/' */
   		length -= cur;
-		strncpy(path + length, kobject_name(parent), cur);
+		memcpy(path + length, kobject_name(parent), cur);
   		*(path + --length) = '/';
   	}