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
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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Stafford Horne <[email protected]>
---
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
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 <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Signed-off-by: Stafford Horne <[email protected]>
---
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
On Fri, Jun 22, 2018 at 7:07 PM, Stafford Horne <[email protected]> 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
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 <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Signed-off-by: Stafford Horne <[email protected]>
> ---
> 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
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 <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Signed-off-by: Stafford Horne <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Signed-off-by: Stafford Horne <[email protected]>
> > ---
> > 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
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