All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Fix GCC -Wstringop-truncation warnings
@ 2018-06-25 12:45 Stafford Horne
  2018-06-25 12:45 ` [PATCH v2 1/2] crypto: Fix " Stafford Horne
  2018-06-25 12:45 ` [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning Stafford Horne
  0 siblings, 2 replies; 9+ messages in thread
From: Stafford Horne @ 2018-06-25 12:45 UTC (permalink / raw)
  To: LKML; +Cc: Greg KH, arnd, Eric Biggers, linux-crypto, Stafford Horne

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(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings
  2018-06-25 12:45 [PATCH v2 0/2] Fix GCC -Wstringop-truncation warnings Stafford Horne
@ 2018-06-25 12:45 ` Stafford Horne
  2018-06-25 12:59   ` Christophe LEROY
  2018-07-08 16:43   ` Herbert Xu
  2018-06-25 12:45 ` [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning Stafford Horne
  1 sibling, 2 replies; 9+ messages in thread
From: Stafford Horne @ 2018-06-25 12:45 UTC (permalink / raw)
  To: LKML
  Cc: Greg KH, arnd, Eric Biggers, linux-crypto, Stafford Horne,
	Max Filippov, Nick Desaulniers, Herbert Xu, David S. Miller

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 <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 crypto/ablkcipher.c | 2 ++
 crypto/blkcipher.c  | 1 +
 2 files changed, 3 insertions(+)

diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index d880a4897159..1edb5000d783 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -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;
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 01c0d4aa2563..dd4dcab3766a 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -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;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning
  2018-06-25 12:45 [PATCH v2 0/2] Fix GCC -Wstringop-truncation warnings Stafford Horne
  2018-06-25 12:45 ` [PATCH v2 1/2] crypto: Fix " Stafford Horne
@ 2018-06-25 12:45 ` Stafford Horne
  2018-06-25 12:57   ` Christophe LEROY
  1 sibling, 1 reply; 9+ messages in thread
From: Stafford Horne @ 2018-06-25 12:45 UTC (permalink / raw)
  To: LKML; +Cc: Greg KH, arnd, Eric Biggers, linux-crypto, 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 <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Eric Biggers <ebiggers3@gmail.com>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 lib/kobject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 18989b5b3b56..e876957743c8 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -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) = '/';
 	}
 
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning
  2018-06-25 12:45 ` [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning Stafford Horne
@ 2018-06-25 12:57   ` Christophe LEROY
  2018-06-25 13:24     ` Stafford Horne
  0 siblings, 1 reply; 9+ messages in thread
From: Christophe LEROY @ 2018-06-25 12:57 UTC (permalink / raw)
  To: Stafford Horne, LKML; +Cc: Greg KH, arnd, Eric Biggers, linux-crypto



Le 25/06/2018 à 14:45, Stafford Horne a écrit :
> 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.

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

Christophe

> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Eric Biggers <ebiggers3@gmail.com>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   lib/kobject.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 18989b5b3b56..e876957743c8 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -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) = '/';
>   	}
>   
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings
  2018-06-25 12:45 ` [PATCH v2 1/2] crypto: Fix " Stafford Horne
@ 2018-06-25 12:59   ` Christophe LEROY
  2018-06-25 13:40     ` Stafford Horne
  2018-07-08 16:43   ` Herbert Xu
  1 sibling, 1 reply; 9+ messages in thread
From: Christophe LEROY @ 2018-06-25 12:59 UTC (permalink / raw)
  To: Stafford Horne, LKML
  Cc: Greg KH, arnd, Eric Biggers, linux-crypto, Max Filippov,
	Nick Desaulniers, Herbert Xu, David S. Miller



Le 25/06/2018 à 14:45, Stafford Horne a écrit :
> 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 <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: Eric Biggers <ebiggers3@gmail.com>
> Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   crypto/ablkcipher.c | 2 ++
>   crypto/blkcipher.c  | 1 +
>   2 files changed, 3 insertions(+)
> 
> diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> index d880a4897159..1edb5000d783 100644
> --- a/crypto/ablkcipher.c
> +++ b/crypto/ablkcipher.c
> @@ -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   ?

> +	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

> +	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
>   
>   	rblkcipher.blocksize = alg->cra_blocksize;
>   	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;
> diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
> index 01c0d4aa2563..dd4dcab3766a 100644
> --- a/crypto/blkcipher.c
> +++ b/crypto/blkcipher.c
> @@ -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;
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning
  2018-06-25 12:57   ` Christophe LEROY
@ 2018-06-25 13:24     ` Stafford Horne
  2018-06-25 13:32       ` Christophe LEROY
  0 siblings, 1 reply; 9+ messages in thread
From: Stafford Horne @ 2018-06-25 13:24 UTC (permalink / raw)
  To: Christophe LEROY; +Cc: LKML, Greg KH, arnd, Eric Biggers, linux-crypto

On Mon, Jun 25, 2018 at 02:57:13PM +0200, Christophe LEROY wrote:
> 
> 
> Le 25/06/2018 à 14:45, Stafford Horne a écrit :
> > 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.
> 
> 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

> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Eric Biggers <ebiggers3@gmail.com>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   lib/kobject.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index 18989b5b3b56..e876957743c8 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -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) = '/';
> >   	}
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning
  2018-06-25 13:24     ` Stafford Horne
@ 2018-06-25 13:32       ` Christophe LEROY
  0 siblings, 0 replies; 9+ messages in thread
From: Christophe LEROY @ 2018-06-25 13:32 UTC (permalink / raw)
  To: Stafford Horne; +Cc: LKML, Greg KH, arnd, Eric Biggers, linux-crypto



Le 25/06/2018 à 15:24, Stafford Horne a écrit :
> On Mon, Jun 25, 2018 at 02:57:13PM +0200, Christophe LEROY wrote:
>>
>>
>> Le 25/06/2018 à 14:45, Stafford Horne a écrit :
>>> 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.
>>
>> 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.

Ok, fair enough.

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> 
> -Stafford
> 
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Eric Biggers <ebiggers3@gmail.com>
>>> Signed-off-by: Stafford Horne <shorne@gmail.com>
>>> ---
>>>    lib/kobject.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/kobject.c b/lib/kobject.c
>>> index 18989b5b3b56..e876957743c8 100644
>>> --- a/lib/kobject.c
>>> +++ b/lib/kobject.c
>>> @@ -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) = '/';
>>>    	}
>>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings
  2018-06-25 12:59   ` Christophe LEROY
@ 2018-06-25 13:40     ` Stafford Horne
  0 siblings, 0 replies; 9+ messages in thread
From: Stafford Horne @ 2018-06-25 13:40 UTC (permalink / raw)
  To: Christophe LEROY
  Cc: LKML, Greg KH, arnd, Eric Biggers, linux-crypto, Max Filippov,
	Nick Desaulniers, Herbert Xu, David S. Miller

On Mon, Jun 25, 2018 at 02:59:58PM +0200, Christophe LEROY wrote:
> 
> 
> Le 25/06/2018 à 14:45, Stafford Horne a écrit :
> > 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 <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Max Filippov <jcmvbkbc@gmail.com>
> > Cc: Eric Biggers <ebiggers3@gmail.com>
> > Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   crypto/ablkcipher.c | 2 ++
> >   crypto/blkcipher.c  | 1 +
> >   2 files changed, 3 insertions(+)
> > 
> > diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> > index d880a4897159..1edb5000d783 100644
> > --- a/crypto/ablkcipher.c
> > +++ b/crypto/ablkcipher.c
> > @@ -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

> > +	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
> 
> > +	rblkcipher.geniv[sizeof(rblkcipher.geniv) - 1] = '\0';
> >   	rblkcipher.blocksize = alg->cra_blocksize;
> >   	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;
> > diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
> > index 01c0d4aa2563..dd4dcab3766a 100644
> > --- a/crypto/blkcipher.c
> > +++ b/crypto/blkcipher.c
> > @@ -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;
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] crypto: Fix -Wstringop-truncation warnings
  2018-06-25 12:45 ` [PATCH v2 1/2] crypto: Fix " Stafford Horne
  2018-06-25 12:59   ` Christophe LEROY
@ 2018-07-08 16:43   ` Herbert Xu
  1 sibling, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2018-07-08 16:43 UTC (permalink / raw)
  To: Stafford Horne
  Cc: LKML, Greg KH, arnd, Eric Biggers, linux-crypto, Max Filippov,
	Nick Desaulniers, David S. Miller

On Mon, Jun 25, 2018 at 09:45:37PM +0900, Stafford Horne wrote:
> 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 <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: Eric Biggers <ebiggers3@gmail.com>
> Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
> Signed-off-by: Stafford Horne <shorne@gmail.com>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-07-08 16:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-25 12:45 [PATCH v2 0/2] Fix GCC -Wstringop-truncation warnings Stafford Horne
2018-06-25 12:45 ` [PATCH v2 1/2] crypto: Fix " Stafford Horne
2018-06-25 12:59   ` Christophe LEROY
2018-06-25 13:40     ` Stafford Horne
2018-07-08 16:43   ` Herbert Xu
2018-06-25 12:45 ` [PATCH v2 2/2] kobject: Fix -Wstringop-truncation warning Stafford Horne
2018-06-25 12:57   ` Christophe LEROY
2018-06-25 13:24     ` Stafford Horne
2018-06-25 13:32       ` Christophe LEROY

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.