| Message ID | 20180623020753.27266-2-shorne@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
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.
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
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
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
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;
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(-)