| Message ID | 20210810063954.628244-1-pizhenwei@bytedance.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: public_key: fix overflow during implicit conversion | expand |
PING On 8/10/21 2:39 PM, zhenwei pi wrote: > Hit kernel warning like this, it can be reproduced by verifying 256 > bytes datafile by keyctl command. > > WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 pkcs1pad_verify+0x160/0x190 > ... > Call Trace: > public_key_verify_signature+0x282/0x380 > ? software_key_query+0x12d/0x180 > ? keyctl_pkey_params_get+0xd6/0x130 > asymmetric_key_verify_signature+0x66/0x80 > keyctl_pkey_verify+0xa5/0x100 > do_syscall_64+0x35/0xb0 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > '.digest_size(u8) = params->in_len(u32)' leads overflow of an u8 value, > so use u32 instead of u8 of digest. And reorder struct > public_key_signature, it could save 8 bytes on a 64 bit machine. > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> > --- > include/crypto/public_key.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h > index 47accec68cb0..f603325c0c30 100644 > --- a/include/crypto/public_key.h > +++ b/include/crypto/public_key.h > @@ -38,9 +38,9 @@ extern void public_key_free(struct public_key *key); > struct public_key_signature { > struct asymmetric_key_id *auth_ids[2]; > u8 *s; /* Signature */ > - u32 s_size; /* Number of bytes in signature */ > u8 *digest; > - u8 digest_size; /* Number of bytes in digest */ > + u32 s_size; /* Number of bytes in signature */ > + u32 digest_size; /* Number of bytes in digest */ > const char *pkey_algo; > const char *hash_algo; > const char *encoding; >
On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: > PING Please, do not top-post. You are lacking Herbert Xu: $ scripts/get_maintainer.pl crypto/asymmetric_keys/public_key.c David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) linux-crypto@vger.kernel.org (open list:CRYPTO API) linux-kernel@vger.kernel.org (open list) > On 8/10/21 2:39 PM, zhenwei pi wrote: > > Hit kernel warning like this, it can be reproduced by verifying 256 > > bytes datafile by keyctl command. > > > > WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 > > pkcs1pad_verify+0x160/0x190 > > ... > > Call Trace: > > public_key_verify_signature+0x282/0x380 > > ? software_key_query+0x12d/0x180 > > ? keyctl_pkey_params_get+0xd6/0x130 > > asymmetric_key_verify_signature+0x66/0x80 > > keyctl_pkey_verify+0xa5/0x100 > > do_syscall_64+0x35/0xb0 > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > '.digest_size(u8) = params->in_len(u32)' leads overflow of an u8 Where is this statement? > > value, > > so use u32 instead of u8 of digest. And reorder struct > > public_key_signature, it could save 8 bytes on a 64 bit machine. ~~~~~ 64-bit What do you mean by "could"? Does it, or does it not? > > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> Nit: "Firstname Lastname" (first letters capitalized) > > --- > > include/crypto/public_key.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/crypto/public_key.h > > b/include/crypto/public_key.h > > index 47accec68cb0..f603325c0c30 100644 > > --- a/include/crypto/public_key.h > > +++ b/include/crypto/public_key.h > > @@ -38,9 +38,9 @@ extern void public_key_free(struct public_key > > *key); > > struct public_key_signature { > > struct asymmetric_key_id *auth_ids[2]; > > u8 *s; /* Signature */ > > - u32 s_size; /* Number of bytes in signature */ > > u8 *digest; > > - u8 digest_size; /* Number of bytes in digest */ > > + u32 s_size; /* Number of bytes in signature */ > > + u32 digest_size; /* Number of bytes in digest */ > > const char *pkey_algo; > > const char *hash_algo; > > const char *encoding; > > /Jarkko
On Wed, Aug 18, 2021 at 03:33:32PM +0300, Jarkko Sakkinen wrote: > On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: > > PING > > Please, do not top-post. > > You are lacking Herbert Xu: I think he already cc'ed me but this patch really belongs to David Howells' tree. Thanks,
On 8/18/21 8:33 PM, Jarkko Sakkinen wrote: > On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: >> PING > > Please, do not top-post. > > You are lacking Herbert Xu: > > $ scripts/get_maintainer.pl crypto/asymmetric_keys/public_key.c > David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) > Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) > "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) > keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) > linux-crypto@vger.kernel.org (open list:CRYPTO API) > linux-kernel@vger.kernel.org (open list) > >> On 8/10/21 2:39 PM, zhenwei pi wrote: >>> Hit kernel warning like this, it can be reproduced by verifying 256 >>> bytes datafile by keyctl command. >>> >>> WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 >>> pkcs1pad_verify+0x160/0x190 >>> ... >>> Call Trace: >>> public_key_verify_signature+0x282/0x380 >>> ? software_key_query+0x12d/0x180 >>> ? keyctl_pkey_params_get+0xd6/0x130 >>> asymmetric_key_verify_signature+0x66/0x80 >>> keyctl_pkey_verify+0xa5/0x100 >>> do_syscall_64+0x35/0xb0 >>> entry_SYSCALL_64_after_hwframe+0x44/0xae >>> >>> '.digest_size(u8) = params->in_len(u32)' leads overflow of an u8 > > Where is this statement? > In function "static int asymmetric_key_verify_signature(struct kernel_pkey_params *params, const void *in, const void *in2)" >>> value, >>> so use u32 instead of u8 of digest. And reorder struct >>> public_key_signature, it could save 8 bytes on a 64 bit machine. > ~~~~~ > 64-bit > > What do you mean by "could"? Does it, or does it > not? > > After reordering struct public_key_signature, sizeof(struct public_key_signature) gets smaller than the original version. > > > >>> >>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> > > Nit: "Firstname Lastname" (first letters capitalized) > >>> --- >>> include/crypto/public_key.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/crypto/public_key.h >>> b/include/crypto/public_key.h >>> index 47accec68cb0..f603325c0c30 100644 >>> --- a/include/crypto/public_key.h >>> +++ b/include/crypto/public_key.h >>> @@ -38,9 +38,9 @@ extern void public_key_free(struct public_key >>> *key); >>> struct public_key_signature { >>> struct asymmetric_key_id *auth_ids[2]; >>> u8 *s; /* Signature */ >>> - u32 s_size; /* Number of bytes in signature */ >>> u8 *digest; >>> - u8 digest_size; /* Number of bytes in digest */ >>> + u32 s_size; /* Number of bytes in signature */ >>> + u32 digest_size; /* Number of bytes in digest */ >>> const char *pkey_algo; >>> const char *hash_algo; >>> const char *encoding; >>> > > /Jarkko >
On Thu, 2021-08-19 at 10:03 +0800, zhenwei pi wrote: > On 8/18/21 8:33 PM, Jarkko Sakkinen wrote: > > On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: > > > PING > > > > Please, do not top-post. > > > > You are lacking Herbert Xu: > > > > $ scripts/get_maintainer.pl crypto/asymmetric_keys/public_key.c > > David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) > > Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) > > "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) > > keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) > > linux-crypto@vger.kernel.org (open list:CRYPTO API) > > linux-kernel@vger.kernel.org (open list) > > > > > On 8/10/21 2:39 PM, zhenwei pi wrote: > > > > Hit kernel warning like this, it can be reproduced by verifying > > > > 256 > > > > bytes datafile by keyctl command. > > > > > > > > WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 > > > > pkcs1pad_verify+0x160/0x190 > > > > ... > > > > Call Trace: > > > > public_key_verify_signature+0x282/0x380 > > > > ? software_key_query+0x12d/0x180 > > > > ? keyctl_pkey_params_get+0xd6/0x130 > > > > asymmetric_key_verify_signature+0x66/0x80 > > > > keyctl_pkey_verify+0xa5/0x100 > > > > do_syscall_64+0x35/0xb0 > > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > > > > > '.digest_size(u8) = params->in_len(u32)' leads overflow of an > > > > u8 > > > > Where is this statement? > > > > In function "static int asymmetric_key_verify_signature(struct > kernel_pkey_params *params, const void *in, const void *in2)" > > > > > value, > > > > so use u32 instead of u8 of digest. And reorder struct > > > > public_key_signature, it could save 8 bytes on a 64 bit > > > > machine. > > ~~~~~ > > 64-bit > > > > What do you mean by "could"? Does it, or does it > > not? > > > > > > > After reordering struct public_key_signature, sizeof(struct > public_key_signature) gets smaller than the original version. OK, then just state is as "it saves" instead of "it could save". Not a requirement but have you been able to trigger this for a kernel that does not have this fix? /Jarkko
On 8/19/21 6:35 PM, Jarkko Sakkinen wrote: > On Thu, 2021-08-19 at 10:03 +0800, zhenwei pi wrote: >> On 8/18/21 8:33 PM, Jarkko Sakkinen wrote: >>> On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: >>>> PING >>> >>> Please, do not top-post. >>> >>> You are lacking Herbert Xu: >>> >>> $ scripts/get_maintainer.pl crypto/asymmetric_keys/public_key.c >>> David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) >>> Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) >>> "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) >>> keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) >>> linux-crypto@vger.kernel.org (open list:CRYPTO API) >>> linux-kernel@vger.kernel.org (open list) >>> >>>> On 8/10/21 2:39 PM, zhenwei pi wrote: >>>>> Hit kernel warning like this, it can be reproduced by verifying >>>>> 256 >>>>> bytes datafile by keyctl command. >>>>> >>>>> WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 >>>>> pkcs1pad_verify+0x160/0x190 >>>>> ... >>>>> Call Trace: >>>>> public_key_verify_signature+0x282/0x380 >>>>> ? software_key_query+0x12d/0x180 >>>>> ? keyctl_pkey_params_get+0xd6/0x130 >>>>> asymmetric_key_verify_signature+0x66/0x80 >>>>> keyctl_pkey_verify+0xa5/0x100 >>>>> do_syscall_64+0x35/0xb0 >>>>> entry_SYSCALL_64_after_hwframe+0x44/0xae >>>>> >>>>> '.digest_size(u8) = params->in_len(u32)' leads overflow of an >>>>> u8 >>> >>> Where is this statement? >>> >> >> In function "static int asymmetric_key_verify_signature(struct >> kernel_pkey_params *params, const void *in, const void *in2)" >> >>>>> value, >>>>> so use u32 instead of u8 of digest. And reorder struct >>>>> public_key_signature, it could save 8 bytes on a 64 bit >>>>> machine. >>> ~~~~~ >>> 64-bit >>> >>> What do you mean by "could"? Does it, or does it >>> not? >>> >>> >>> >> After reordering struct public_key_signature, sizeof(struct >> public_key_signature) gets smaller than the original version. > > OK, then just state is as "it saves" instead of "it could save". > > Not a requirement but have you been able to trigger this for a > kernel that does not have this fix? > This kernel warning can be reproduced on debian11(Linux-5.10.0-8-amd64) by the following script: RAWDATA=rawdata SIGDATA=sigdata modprobe pkcs8_key_parser rm -rf *.der *.pem *.pfx rm -rf $RAWDATA dd if=/dev/random of=$RAWDATA bs=256 count=1 openssl req -nodes -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -subj "/C=CN/ST=GD/L=SZ/O=vihoo/OU=dev/CN=xx.com/emailAddress=yy@xx.com" KEY_ID=`openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER | keyctl padd asymmetric 123 @s` keyctl pkey_sign $KEY_ID 0 $RAWDATA enc=pkcs1 hash=sha1 > $SIGDATA keyctl pkey_verify $KEY_ID 0 $RAWDATA $SIGDATA enc=pkcs1 hash=sha1 > /Jarkko >
On Thu, 2021-08-19 at 18:52 +0800, zhenwei pi wrote: > On 8/19/21 6:35 PM, Jarkko Sakkinen wrote: > > On Thu, 2021-08-19 at 10:03 +0800, zhenwei pi wrote: > > > On 8/18/21 8:33 PM, Jarkko Sakkinen wrote: > > > > On Wed, 2021-08-18 at 16:33 +0800, zhenwei pi wrote: > > > > > PING > > > > > > > > Please, do not top-post. > > > > > > > > You are lacking Herbert Xu: > > > > > > > > $ scripts/get_maintainer.pl crypto/asymmetric_keys/public_key.c > > > > David Howells <dhowells@redhat.com> (maintainer:ASYMMETRIC KEYS) > > > > Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API) > > > > "David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API) > > > > keyrings@vger.kernel.org (open list:ASYMMETRIC KEYS) > > > > linux-crypto@vger.kernel.org (open list:CRYPTO API) > > > > linux-kernel@vger.kernel.org (open list) > > > > > > > > > On 8/10/21 2:39 PM, zhenwei pi wrote: > > > > > > Hit kernel warning like this, it can be reproduced by verifying > > > > > > 256 > > > > > > bytes datafile by keyctl command. > > > > > > > > > > > > WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 > > > > > > pkcs1pad_verify+0x160/0x190 > > > > > > ... > > > > > > Call Trace: > > > > > > public_key_verify_signature+0x282/0x380 > > > > > > ? software_key_query+0x12d/0x180 > > > > > > ? keyctl_pkey_params_get+0xd6/0x130 > > > > > > asymmetric_key_verify_signature+0x66/0x80 > > > > > > keyctl_pkey_verify+0xa5/0x100 > > > > > > do_syscall_64+0x35/0xb0 > > > > > > entry_SYSCALL_64_after_hwframe+0x44/0xae > > > > > > > > > > > > '.digest_size(u8) = params->in_len(u32)' leads overflow of an > > > > > > u8 > > > > > > > > Where is this statement? > > > > > > > > > > In function "static int asymmetric_key_verify_signature(struct > > > kernel_pkey_params *params, const void *in, const void *in2)" > > > > > > > > > value, > > > > > > so use u32 instead of u8 of digest. And reorder struct > > > > > > public_key_signature, it could save 8 bytes on a 64 bit > > > > > > machine. > > > > ~~~~~ > > > > 64-bit > > > > > > > > What do you mean by "could"? Does it, or does it > > > > not? > > > > > > > > > > > > > > > After reordering struct public_key_signature, sizeof(struct > > > public_key_signature) gets smaller than the original version. > > > > OK, then just state is as "it saves" instead of "it could save". > > > > Not a requirement but have you been able to trigger this for a > > kernel that does not have this fix? > > > This kernel warning can be reproduced on debian11(Linux-5.10.0-8-amd64) > by the following script: > > RAWDATA=rawdata > SIGDATA=sigdata > > modprobe pkcs8_key_parser > > rm -rf *.der *.pem *.pfx > rm -rf $RAWDATA > dd if=/dev/random of=$RAWDATA bs=256 count=1 > > openssl req -nodes -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem > -subj "/C=CN/ST=GD/L=SZ/O=vihoo/OU=dev/CN=xx.com/emailAddress=yy@xx.com" > > KEY_ID=`openssl pkcs8 -in key.pem -topk8 -nocrypt -outform DER | keyctl > padd asymmetric 123 @s` > > keyctl pkey_sign $KEY_ID 0 $RAWDATA enc=pkcs1 hash=sha1 > $SIGDATA > keyctl pkey_verify $KEY_ID 0 $RAWDATA $SIGDATA enc=pkcs1 hash=sha1 Thank you. I'll see if I can reproduce this when you send a new version (if not, it is not constraint for accepting to patch, but I'll still try). PS. Ignore the firstname lastname comment. I was not aware that in some cultures it is written like that (James Bottomley pointed this out). /Jarkko
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h index 47accec68cb0..f603325c0c30 100644 --- a/include/crypto/public_key.h +++ b/include/crypto/public_key.h @@ -38,9 +38,9 @@ extern void public_key_free(struct public_key *key); struct public_key_signature { struct asymmetric_key_id *auth_ids[2]; u8 *s; /* Signature */ - u32 s_size; /* Number of bytes in signature */ u8 *digest; - u8 digest_size; /* Number of bytes in digest */ + u32 s_size; /* Number of bytes in signature */ + u32 digest_size; /* Number of bytes in digest */ const char *pkey_algo; const char *hash_algo; const char *encoding;
Hit kernel warning like this, it can be reproduced by verifying 256 bytes datafile by keyctl command. WARNING: CPU: 5 PID: 344556 at crypto/rsa-pkcs1pad.c:540 pkcs1pad_verify+0x160/0x190 ... Call Trace: public_key_verify_signature+0x282/0x380 ? software_key_query+0x12d/0x180 ? keyctl_pkey_params_get+0xd6/0x130 asymmetric_key_verify_signature+0x66/0x80 keyctl_pkey_verify+0xa5/0x100 do_syscall_64+0x35/0xb0 entry_SYSCALL_64_after_hwframe+0x44/0xae '.digest_size(u8) = params->in_len(u32)' leads overflow of an u8 value, so use u32 instead of u8 of digest. And reorder struct public_key_signature, it could save 8 bytes on a 64 bit machine. Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> --- include/crypto/public_key.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)