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
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(+)
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
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(-)
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
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 <[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(-)
>
> 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) = '/';
> }
>
>
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 <[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(+)
>
> 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;
>
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 <[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(-)
> >
> > 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) = '/';
> > }
> >
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 <[email protected]>
>
> -Stafford
>
>>>
>>> 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(-)
>>>
>>> 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) = '/';
>>> }
>>>
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 <[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(+)
> >
> > 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;
> >
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 <[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.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt