[RFC PATCH 0/2] Fix GCC -Wstringop-truncation warnings

[RFC PATCH 0/2] Fix GCC -Wstringop-truncation warnings

[Stafford Horne]

Hello,

When compiling OpenRISC kernels with our new toolchain based on 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 yesterday and it seems no
one has pointed these out yet, so here are the patches.

Please let me know if I should keep these on the OpenRISC tree (which I can do
for 4.18) or if the maintainers will pick them up separately.

-Stafford

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

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

-- 
2.17.0

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

[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
limiting the size of the string copy to include the null terminator.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 crypto/ablkcipher.c | 4 ++--
 crypto/blkcipher.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index d880a4897159..972cd7c879f6 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -372,7 +372,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));
+		sizeof(rblkcipher.geniv) - 1);
 
 	rblkcipher.blocksize = alg->cra_blocksize;
 	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;
@@ -446,7 +446,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));
+		sizeof(rblkcipher.geniv) - 1);
 
 	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..f1644b5cf68c 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -511,7 +511,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));
+		sizeof(rblkcipher.geniv) - 1);
 
 	rblkcipher.blocksize = alg->cra_blocksize;
 	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;
-- 
2.17.0

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

[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 pointing out a bug that the strncpy limit is the source string not the
destination buffer remaining length.  Fix it.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
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..15338e5a96f2 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);
+		strncpy(path + length, kobject_name(parent), length);
 		*(path + --length) = '/';
 	}
 
-- 
2.17.0

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

[Max Filippov]

On Fri, Jun 22, 2018 at 7:07 PM, Stafford Horne <shorne@gmail.com> 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
> limiting the size of the string copy to include the null terminator.

That could work if the destination buffer was zero-initialized,
but it's allocated on stack and is not initialized.
Replacing strncpy with strlcpy without changing its arguments
should do the right thing.

-- 
Thanks.
-- Max

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

[Eric Biggers]

On Sat, Jun 23, 2018 at 11:07:53AM +0900, Stafford Horne wrote:
> 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 pointing out a bug that the strncpy limit is the source string not the
> destination buffer remaining length.  Fix it.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> 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..15338e5a96f2 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);
> +		strncpy(path + length, kobject_name(parent), length);
>  		*(path + --length) = '/';
>  	}

It should be replaced with memcpy(), AFAICS; it wouldn't change the behavior but
it would quiet the gcc warning.  Your proposed "fix" is heavily broken: notice
that the code is building a string backwards (end to beginning).

- Eric

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

[Eric Biggers]

On Sat, Jun 23, 2018 at 11:07:52AM +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
> limiting the size of the string copy to include the null terminator.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>  crypto/ablkcipher.c | 4 ++--
>  crypto/blkcipher.c  | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> index d880a4897159..972cd7c879f6 100644
> --- a/crypto/ablkcipher.c
> +++ b/crypto/ablkcipher.c
> @@ -372,7 +372,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));
> +		sizeof(rblkcipher.geniv) - 1);
>  
>  	rblkcipher.blocksize = alg->cra_blocksize;
>  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;
> @@ -446,7 +446,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));
> +		sizeof(rblkcipher.geniv) - 1);
>  
>  	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..f1644b5cf68c 100644
> --- a/crypto/blkcipher.c
> +++ b/crypto/blkcipher.c
> @@ -511,7 +511,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));
> +		sizeof(rblkcipher.geniv) - 1);
>  
>  	rblkcipher.blocksize = alg->cra_blocksize;
>  	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;

Your "fix" introduces an information disclosure bug, as it results in
uninitialized memory being copied to userspace.  This same broken patch was sent
by someone else too.

Maybe it would be best to just memset() the crypto_report_* structs to 0 after
declaration and then replace the strncpy()'s with strscpy()'s, even if just to
stop people from sending broken "fixes".  Do you want to do that?

- Eric

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

[Stafford Horne]

On Fri, Jun 22, 2018 at 07:31:49PM -0700, Eric Biggers wrote:
> On Sat, Jun 23, 2018 at 11:07:53AM +0900, Stafford Horne wrote:
> > 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 pointing out a bug that the strncpy limit is the source string not the
> > destination buffer remaining length.  Fix it.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > 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..15338e5a96f2 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);
> > +		strncpy(path + length, kobject_name(parent), length);
> >  		*(path + --length) = '/';
> >  	}
> 
> It should be replaced with memcpy(), AFAICS; it wouldn't change the behavior but
> it would quiet the gcc warning.  Your proposed "fix" is heavily broken: notice
> that the code is building a string backwards (end to beginning).

Sorry about that, I didnt notice.  I will fix.

-Stafford

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

[Stafford Horne]

On Fri, Jun 22, 2018 at 07:41:49PM -0700, Eric Biggers wrote:
> On Sat, Jun 23, 2018 at 11:07:52AM +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
> > limiting the size of the string copy to include the null terminator.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >  crypto/ablkcipher.c | 4 ++--
> >  crypto/blkcipher.c  | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
> > index d880a4897159..972cd7c879f6 100644
> > --- a/crypto/ablkcipher.c
> > +++ b/crypto/ablkcipher.c
> > @@ -372,7 +372,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));
> > +		sizeof(rblkcipher.geniv) - 1);
> >  
> >  	rblkcipher.blocksize = alg->cra_blocksize;
> >  	rblkcipher.min_keysize = alg->cra_ablkcipher.min_keysize;
> > @@ -446,7 +446,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));
> > +		sizeof(rblkcipher.geniv) - 1);
> >  
> >  	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..f1644b5cf68c 100644
> > --- a/crypto/blkcipher.c
> > +++ b/crypto/blkcipher.c
> > @@ -511,7 +511,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));
> > +		sizeof(rblkcipher.geniv) - 1);
> >  
> >  	rblkcipher.blocksize = alg->cra_blocksize;
> >  	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;
> 
> Your "fix" introduces an information disclosure bug, as it results in
> uninitialized memory being copied to userspace.  This same broken patch was sent
> by someone else too.
> 
> Maybe it would be best to just memset() the crypto_report_* structs to 0 after
> declaration and then replace the strncpy()'s with strscpy()'s, even if just to
> stop people from sending broken "fixes".  Do you want to do that?

Right, I didnt realize that we were using strncpy to also init the whole buffer.

I will do as suggest, and respic.

-Stafford

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

[Stafford Horne]

On Sat, Jun 23, 2018 at 11:52:56AM +0900, Stafford Horne wrote:
> On Fri, Jun 22, 2018 at 07:41:49PM -0700, Eric Biggers wrote:
> > On Sat, Jun 23, 2018 at 11:07:52AM +0900, Stafford Horne wrote:
> > > As of GCC 9.0.0 the build is reporting warnings like:
> > > 
[...]
> > >  	strncpy(rblkcipher.type, "blkcipher", sizeof(rblkcipher.type));
> > >  	strncpy(rblkcipher.geniv, alg->cra_blkcipher.geniv ?: "<default>",
> > > -		sizeof(rblkcipher.geniv));
> > > +		sizeof(rblkcipher.geniv) - 1);
> > >  
> > >  	rblkcipher.blocksize = alg->cra_blocksize;
> > >  	rblkcipher.min_keysize = alg->cra_blkcipher.min_keysize;
> > 
> > Your "fix" introduces an information disclosure bug, as it results in
> > uninitialized memory being copied to userspace.  This same broken patch was sent
> > by someone else too.
> > 
> > Maybe it would be best to just memset() the crypto_report_* structs to 0 after
> > declaration and then replace the strncpy()'s with strscpy()'s, even if just to
> > stop people from sending broken "fixes".  Do you want to do that?
> 
> Right, I didnt realize that we were using strncpy to also init the whole buffer.
> 
> I will do as suggest, and respin.

Hi Eric,

I thought about this a bit, doing memset() and strscpy() seemed fine, but the
below also would work, be a bit faster and stop gcc form complaining.  What do
you think?

@@ -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;

Let me know what you think.

-Stafford