* [PATCH v5 0/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() @ 2022-12-27 14:27 Roberto Sassu 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu 2022-12-27 14:27 ` [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 0 siblings, 2 replies; 24+ messages in thread From: Roberto Sassu @ 2022-12-27 14:27 UTC (permalink / raw) To: dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu From: Roberto Sassu <roberto.sassu@huawei.com> Changelog: v4: - Replace sg_init_table()/sg_set_buf() with sg_init_one() (suggested by Eric) v3: v2: - Add patch by Herbert to take only the needed bytes for a MPI from the scatterlist - Use only one scatterlist for signature and digest (suggested by Eric) - Rename key variable to buf (suggested by Eric) - Rename key_max_len variable to buf_len - Use size_t for the buf_len variable instead of u32 v1: - Unconditionally copy the signature and digest to the buffer to keep the code simple (suggested by Eric) Herbert Xu (1): lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu (1): KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- lib/mpi/mpicoder.c | 3 ++- 2 files changed, 23 insertions(+), 18 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-27 14:27 [PATCH v5 0/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu @ 2022-12-27 14:27 ` Roberto Sassu 2022-12-29 22:38 ` Eric Biggers ` (2 more replies) 2022-12-27 14:27 ` [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 1 sibling, 3 replies; 24+ messages in thread From: Roberto Sassu @ 2022-12-27 14:27 UTC (permalink / raw) To: dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu From: Herbert Xu <herbert@gondor.apana.org.au> The helper mpi_read_raw_from_sgl sets the number of entries in the SG list according to nbytes. However, if the last entry in the SG list contains more data than nbytes, then it may overrun the buffer because it only allocates enough memory for nbytes. Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- lib/mpi/mpicoder.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c index 39c4c6731094..3cb6bd148fa9 100644 --- a/lib/mpi/mpicoder.c +++ b/lib/mpi/mpicoder.c @@ -504,7 +504,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes) while (sg_miter_next(&miter)) { buff = miter.addr; - len = miter.length; + len = min_t(unsigned, miter.length, nbytes); + nbytes -= len; for (x = 0; x < len; x++) { a <<= 8; -- 2.25.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu @ 2022-12-29 22:38 ` Eric Biggers 2022-12-30 13:35 ` David Laight 2023-01-06 15:18 ` Herbert Xu 2 siblings, 0 replies; 24+ messages in thread From: Eric Biggers @ 2022-12-29 22:38 UTC (permalink / raw) To: Roberto Sassu Cc: dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > > The helper mpi_read_raw_from_sgl sets the number of entries in > the SG list according to nbytes. However, if the last entry > in the SG list contains more data than nbytes, then it may overrun > the buffer because it only allocates enough memory for nbytes. > > Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") > Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > lib/mpi/mpicoder.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu 2022-12-29 22:38 ` Eric Biggers @ 2022-12-30 13:35 ` David Laight 2022-12-30 15:39 ` Herbert Xu 2023-01-06 15:18 ` Herbert Xu 2 siblings, 1 reply; 24+ messages in thread From: David Laight @ 2022-12-30 13:35 UTC (permalink / raw) To: 'Roberto Sassu', dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable From: Roberto Sassu > Sent: 27 December 2022 14:28 > > From: Herbert Xu <herbert@gondor.apana.org.au> > > The helper mpi_read_raw_from_sgl sets the number of entries in > the SG list according to nbytes. However, if the last entry > in the SG list contains more data than nbytes, then it may overrun > the buffer because it only allocates enough memory for nbytes. > > Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") > Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > lib/mpi/mpicoder.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c > index 39c4c6731094..3cb6bd148fa9 100644 > --- a/lib/mpi/mpicoder.c > +++ b/lib/mpi/mpicoder.c > @@ -504,7 +504,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes) > > while (sg_miter_next(&miter)) { > buff = miter.addr; > - len = miter.length; > + len = min_t(unsigned, miter.length, nbytes); Technically that min_t() is incorrect. miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int. Any cast needs to force the smaller type to the larger one. (Clearly here the domain of the values is probably than 4G - but that isn't the point. There must be some places where the sg length needs to be size_t because 32 bits isn't enough.) In reality min() is being completely over-zealous in its checking and should allow comparisons where the signed-ness of the two values matches. Search for the patch I posted before xmas. David > + nbytes -= len; > > for (x = 0; x < len; x++) { > a <<= 8; > -- > 2.25.1 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-30 13:35 ` David Laight @ 2022-12-30 15:39 ` Herbert Xu 2022-12-31 13:23 ` David Laight 0 siblings, 1 reply; 24+ messages in thread From: Herbert Xu @ 2022-12-30 15:39 UTC (permalink / raw) To: David Laight Cc: 'Roberto Sassu', dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On Fri, Dec 30, 2022 at 01:35:07PM +0000, David Laight wrote: > > miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int. miter.length is bounded by sg->length which is unsigned int. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-30 15:39 ` Herbert Xu @ 2022-12-31 13:23 ` David Laight 0 siblings, 0 replies; 24+ messages in thread From: David Laight @ 2022-12-31 13:23 UTC (permalink / raw) To: 'Herbert Xu' Cc: 'Roberto Sassu', dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable From: Herbert Xu > Sent: 30 December 2022 15:40 > > On Fri, Dec 30, 2022 at 01:35:07PM +0000, David Laight wrote: > > > > miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int. > > miter.length is bounded by sg->length which is unsigned int. I did say 'technically' :-) Should there be a sg_miter_stop() before the return at the bottom? Care seems to have been taken to add one before an earlier error return. (The logic in that function is very strange...) Indeed other parts of the file are equally strange. The big multi-line if-else in twocompl() is just: p[i] = (p[1] ^ 0xff) + 1; or even: p[i] = -p[i]; That function could also return the 'zero status' to correct for -0 (rather than the extra check earlier in the caller). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu 2022-12-29 22:38 ` Eric Biggers 2022-12-30 13:35 ` David Laight @ 2023-01-06 15:18 ` Herbert Xu 2023-01-16 8:57 ` Roberto Sassu 2 siblings, 1 reply; 24+ messages in thread From: Herbert Xu @ 2023-01-06 15:18 UTC (permalink / raw) To: Roberto Sassu Cc: dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote: > From: Herbert Xu <herbert@gondor.apana.org.au> > > The helper mpi_read_raw_from_sgl sets the number of entries in > the SG list according to nbytes. However, if the last entry > in the SG list contains more data than nbytes, then it may overrun > the buffer because it only allocates enough memory for nbytes. > > Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") > Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > lib/mpi/mpicoder.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2023-01-06 15:18 ` Herbert Xu @ 2023-01-16 8:57 ` Roberto Sassu 2023-01-16 9:06 ` Herbert Xu 0 siblings, 1 reply; 24+ messages in thread From: Roberto Sassu @ 2023-01-16 8:57 UTC (permalink / raw) To: Herbert Xu Cc: dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On Fri, 2023-01-06 at 23:18 +0800, Herbert Xu wrote: > On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote: > > From: Herbert Xu <herbert@gondor.apana.org.au> > > > > The helper mpi_read_raw_from_sgl sets the number of entries in > > the SG list according to nbytes. However, if the last entry > > in the SG list contains more data than nbytes, then it may overrun > > the buffer because it only allocates enough memory for nbytes. > > > > Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers") > > Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > --- > > lib/mpi/mpicoder.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > Patch applied. Thanks. Hi Herbert will you take also the second patch? Thanks Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2023-01-16 8:57 ` Roberto Sassu @ 2023-01-16 9:06 ` Herbert Xu 2023-01-20 10:23 ` Roberto Sassu 0 siblings, 1 reply; 24+ messages in thread From: Herbert Xu @ 2023-01-16 9:06 UTC (permalink / raw) To: Roberto Sassu Cc: dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On Mon, Jan 16, 2023 at 09:57:57AM +0100, Roberto Sassu wrote: > > Hi Herbert > > will you take also the second patch? That's part of David Howells' tree so hopefully he will pick it up soon. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long 2023-01-16 9:06 ` Herbert Xu @ 2023-01-20 10:23 ` Roberto Sassu 0 siblings, 0 replies; 24+ messages in thread From: Roberto Sassu @ 2023-01-20 10:23 UTC (permalink / raw) To: Herbert Xu Cc: dhowells, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable On 1/16/2023 10:06 AM, Herbert Xu wrote: > On Mon, Jan 16, 2023 at 09:57:57AM +0100, Roberto Sassu wrote: >> >> Hi Herbert >> >> will you take also the second patch? > > That's part of David Howells' tree so hopefully he will pick > it up soon. Hi David could you please take the second patch? Thanks Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2022-12-27 14:27 [PATCH v5 0/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu @ 2022-12-27 14:27 ` Roberto Sassu 2022-12-29 22:39 ` Eric Biggers 2023-06-01 21:00 ` Stefan Berger 1 sibling, 2 replies; 24+ messages in thread From: Roberto Sassu @ 2022-12-27 14:27 UTC (permalink / raw) To: dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu From: Roberto Sassu <roberto.sassu@huawei.com> Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear mapping") checks that both the signature and the digest reside in the linear mapping area. However, more recently commit ba14a194a434c ("fork: Add generic vmalloced stack support") made it possible to move the stack in the vmalloc area, which is not contiguous, and thus not suitable for sg_set_buf() which needs adjacent pages. Always make a copy of the signature and digest in the same buffer used to store the key and its parameters, and pass them to sg_init_one(). Prefer it to conditionally doing the copy if necessary, to keep the code simple. The buffer allocated with kmalloc() is in the linear mapping area. Cc: stable@vger.kernel.org # 4.9.x Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ Suggested-by: Eric Biggers <ebiggers@kernel.org> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index 2f8352e88860..49a3f7c01149 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey, struct crypto_wait cwait; struct crypto_akcipher *tfm; struct akcipher_request *req; - struct scatterlist src_sg[2]; + struct scatterlist src_sg; char alg_name[CRYPTO_MAX_ALG_NAME]; - char *key, *ptr; + char *buf, *ptr; + size_t buf_len; int ret; pr_devel("==>%s()\n", __func__); @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey, if (!req) goto error_free_tfm; - key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, - GFP_KERNEL); - if (!key) + buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, + sig->s_size + sig->digest_size); + + buf = kmalloc(buf_len, GFP_KERNEL); + if (!buf) goto error_free_req; - memcpy(key, pkey->key, pkey->keylen); - ptr = key + pkey->keylen; + memcpy(buf, pkey->key, pkey->keylen); + ptr = buf + pkey->keylen; ptr = pkey_pack_u32(ptr, pkey->algo); ptr = pkey_pack_u32(ptr, pkey->paramlen); memcpy(ptr, pkey->params, pkey->paramlen); if (pkey->key_is_private) - ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen); + ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen); else - ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen); + ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen); if (ret) - goto error_free_key; + goto error_free_buf; if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) { ret = cert_sig_digest_update(sig, tfm); if (ret) - goto error_free_key; + goto error_free_buf; } - sg_init_table(src_sg, 2); - sg_set_buf(&src_sg[0], sig->s, sig->s_size); - sg_set_buf(&src_sg[1], sig->digest, sig->digest_size); - akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size, + memcpy(buf, sig->s, sig->s_size); + memcpy(buf + sig->s_size, sig->digest, sig->digest_size); + + sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size); + akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size, sig->digest_size); crypto_init_wait(&cwait); akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG | @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey, crypto_req_done, &cwait); ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait); -error_free_key: - kfree(key); +error_free_buf: + kfree(buf); error_free_req: akcipher_request_free(req); error_free_tfm: -- 2.25.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2022-12-27 14:27 ` [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu @ 2022-12-29 22:39 ` Eric Biggers 2023-01-27 8:27 ` Roberto Sassu 2023-06-01 21:00 ` Stefan Berger 1 sibling, 1 reply; 24+ messages in thread From: Eric Biggers @ 2022-12-29 22:39 UTC (permalink / raw) To: Roberto Sassu Cc: dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") checks that both the signature and the digest reside in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support") made it possible to move the stack in the vmalloc area, > which is not contiguous, and thus not suitable for sg_set_buf() which needs > adjacent pages. > > Always make a copy of the signature and digest in the same buffer used to > store the key and its parameters, and pass them to sg_init_one(). Prefer it > to conditionally doing the copy if necessary, to keep the code simple. The > buffer allocated with kmalloc() is in the linear mapping area. > > Cc: stable@vger.kernel.org # 4.9.x > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > Suggested-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > 1 file changed, 21 insertions(+), 17 deletions(-) Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2022-12-29 22:39 ` Eric Biggers @ 2023-01-27 8:27 ` Roberto Sassu 2023-02-09 10:49 ` Roberto Sassu 0 siblings, 1 reply; 24+ messages in thread From: Roberto Sassu @ 2023-01-27 8:27 UTC (permalink / raw) To: dhowells Cc: Eric Biggers, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > mapping") checks that both the signature and the digest reside in the > > linear mapping area. > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > stack support") made it possible to move the stack in the vmalloc area, > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > adjacent pages. > > > > Always make a copy of the signature and digest in the same buffer used to > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > to conditionally doing the copy if necessary, to keep the code simple. The > > buffer allocated with kmalloc() is in the linear mapping area. > > > > Cc: stable@vger.kernel.org # 4.9.x > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > 1 file changed, 21 insertions(+), 17 deletions(-) > > Reviewed-by: Eric Biggers <ebiggers@google.com> Hi David could you please take this patch in your repo, if it is ok? Thanks Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-01-27 8:27 ` Roberto Sassu @ 2023-02-09 10:49 ` Roberto Sassu 2023-02-09 18:53 ` Eric Biggers 0 siblings, 1 reply; 24+ messages in thread From: Roberto Sassu @ 2023-02-09 10:49 UTC (permalink / raw) To: dhowells Cc: Eric Biggers, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > mapping") checks that both the signature and the digest reside in the > > > linear mapping area. > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > stack support") made it possible to move the stack in the vmalloc area, > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > adjacent pages. > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > --- > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > Hi David > > could you please take this patch in your repo, if it is ok? Kindly ask your support here. Has this patch been queued somewhere? Wasn't able to find it, also it is not in linux-next. Thanks Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-09 10:49 ` Roberto Sassu @ 2023-02-09 18:53 ` Eric Biggers 2023-02-09 22:46 ` Paul Moore 2023-02-10 3:52 ` Jarkko Sakkinen 0 siblings, 2 replies; 24+ messages in thread From: Eric Biggers @ 2023-02-09 18:53 UTC (permalink / raw) To: Roberto Sassu, David Howells, Jarkko Sakkinen, Herbert Xu Cc: davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > mapping") checks that both the signature and the digest reside in the > > > > linear mapping area. > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > adjacent pages. > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > --- > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > Hi David > > > > could you please take this patch in your repo, if it is ok? > > Kindly ask your support here. Has this patch been queued somewhere? > Wasn't able to find it, also it is not in linux-next. > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so you'll need to find someone else to apply it. Herbert Xu, the maintainer of the crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some reason; should that change?). - Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-09 18:53 ` Eric Biggers @ 2023-02-09 22:46 ` Paul Moore 2023-02-10 3:56 ` Jarkko Sakkinen 2023-02-10 3:52 ` Jarkko Sakkinen 1 sibling, 1 reply; 24+ messages in thread From: Paul Moore @ 2023-02-09 22:46 UTC (permalink / raw) To: David Howells Cc: Roberto Sassu, Jarkko Sakkinen, Herbert Xu, davem, zohar, dmitry.kasatkin, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu, Eric Biggers On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > mapping") checks that both the signature and the digest reside in the > > > > > linear mapping area. > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > > adjacent pages. > > > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > --- > > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > Hi David > > > > > > could you please take this patch in your repo, if it is ok? > > > > Kindly ask your support here. Has this patch been queued somewhere? > > Wasn't able to find it, also it is not in linux-next. > > > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so > you'll need to find someone else to apply it. Herbert Xu, the maintainer of the > crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some > reason; should that change?). It is problematic that David isn't replying to this. I have no idea if it will work, but I just reached out to him to see if I can draw his attention back to this ... -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-09 22:46 ` Paul Moore @ 2023-02-10 3:56 ` Jarkko Sakkinen 2023-02-10 17:32 ` Paul Moore 0 siblings, 1 reply; 24+ messages in thread From: Jarkko Sakkinen @ 2023-02-10 3:56 UTC (permalink / raw) To: Paul Moore Cc: David Howells, Roberto Sassu, Herbert Xu, davem, zohar, dmitry.kasatkin, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu, Eric Biggers On Thu, Feb 09, 2023 at 05:46:32PM -0500, Paul Moore wrote: > On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > mapping") checks that both the signature and the digest reside in the > > > > > > linear mapping area. > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > > > adjacent pages. > > > > > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > --- > > > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > > > Hi David > > > > > > > > could you please take this patch in your repo, if it is ok? > > > > > > Kindly ask your support here. Has this patch been queued somewhere? > > > Wasn't able to find it, also it is not in linux-next. > > > > > > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so > > you'll need to find someone else to apply it. Herbert Xu, the maintainer of the > > crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who > > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some > > reason; should that change?). > > It is problematic that David isn't replying to this. I have no idea > if it will work, but I just reached out to him to see if I can draw > his attention back to this ... See my response to Eric. BR, Jarkko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-10 3:56 ` Jarkko Sakkinen @ 2023-02-10 17:32 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2023-02-10 17:32 UTC (permalink / raw) To: Jarkko Sakkinen Cc: David Howells, Roberto Sassu, Herbert Xu, davem, zohar, dmitry.kasatkin, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu, Eric Biggers On Thu, Feb 9, 2023 at 10:56 PM Jarkko Sakkinen <jarkko@kernel.org> wrote: > On Thu, Feb 09, 2023 at 05:46:32PM -0500, Paul Moore wrote: > > On Thu, Feb 9, 2023 at 1:53 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > > > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > > mapping") checks that both the signature and the digest reside in the > > > > > > > linear mapping area. > > > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > > > > adjacent pages. > > > > > > > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > --- > > > > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > > > > > Hi David > > > > > > > > > > could you please take this patch in your repo, if it is ok? > > > > > > > > Kindly ask your support here. Has this patch been queued somewhere? > > > > Wasn't able to find it, also it is not in linux-next. > > > > > > > > > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so > > > you'll need to find someone else to apply it. Herbert Xu, the maintainer of the > > > crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who > > > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some > > > reason; should that change?). > > > > It is problematic that David isn't replying to this. I have no idea > > if it will work, but I just reached out to him to see if I can draw > > his attention back to this ... > > See my response to Eric. Thanks Jarkko. -- paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-09 18:53 ` Eric Biggers 2023-02-09 22:46 ` Paul Moore @ 2023-02-10 3:52 ` Jarkko Sakkinen 2023-05-26 7:35 ` Roberto Sassu 1 sibling, 1 reply; 24+ messages in thread From: Jarkko Sakkinen @ 2023-02-10 3:52 UTC (permalink / raw) To: Eric Biggers Cc: Roberto Sassu, David Howells, Herbert Xu, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Thu, Feb 09, 2023 at 06:53:37PM +0000, Eric Biggers wrote: > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > mapping") checks that both the signature and the digest reside in the > > > > > linear mapping area. > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > > adjacent pages. > > > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > --- > > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > Hi David > > > > > > could you please take this patch in your repo, if it is ok? > > > > Kindly ask your support here. Has this patch been queued somewhere? > > Wasn't able to find it, also it is not in linux-next. > > > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so > you'll need to find someone else to apply it. Herbert Xu, the maintainer of the > crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some > reason; should that change?). I can apply this if no objections? BR, Jarkko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-02-10 3:52 ` Jarkko Sakkinen @ 2023-05-26 7:35 ` Roberto Sassu 0 siblings, 0 replies; 24+ messages in thread From: Roberto Sassu @ 2023-05-26 7:35 UTC (permalink / raw) To: Jarkko Sakkinen, Eric Biggers Cc: David Howells, Herbert Xu, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Fri, 2023-02-10 at 05:52 +0200, Jarkko Sakkinen wrote: > On Thu, Feb 09, 2023 at 06:53:37PM +0000, Eric Biggers wrote: > > On Thu, Feb 09, 2023 at 11:49:19AM +0100, Roberto Sassu wrote: > > > On Fri, 2023-01-27 at 09:27 +0100, Roberto Sassu wrote: > > > > On Thu, 2022-12-29 at 14:39 -0800, Eric Biggers wrote: > > > > > On Tue, Dec 27, 2022 at 03:27:40PM +0100, Roberto Sassu wrote: > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > > > mapping") checks that both the signature and the digest reside in the > > > > > > linear mapping area. > > > > > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > > > adjacent pages. > > > > > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > --- > > > > > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > > > > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > > > > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > > > Hi David > > > > > > > > could you please take this patch in your repo, if it is ok? > > > > > > Kindly ask your support here. Has this patch been queued somewhere? > > > Wasn't able to find it, also it is not in linux-next. > > > > > > > The maintainer of asymmetric_keys (David Howells) is ignoring this patch, so > > you'll need to find someone else to apply it. Herbert Xu, the maintainer of the > > crypto subsystem, might be willing to apply it. Or maybe Jarkko Sakkinen, who > > is a co-maintainer of the keyrings subsystem (but not asymmetric_keys, for some > > reason; should that change?). > > I can apply this if no objections? Hi Jarkko I wasn't able to reach David about this patch. Could you please apply it? Thanks Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2022-12-27 14:27 ` [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 2022-12-29 22:39 ` Eric Biggers @ 2023-06-01 21:00 ` Stefan Berger 2023-06-02 9:17 ` Roberto Sassu 1 sibling, 1 reply; 24+ messages in thread From: Stefan Berger @ 2023-06-01 21:00 UTC (permalink / raw) To: Roberto Sassu, dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On 12/27/22 09:27, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > mapping") checks that both the signature and the digest reside in the > linear mapping area. > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > stack support") made it possible to move the stack in the vmalloc area, > which is not contiguous, and thus not suitable for sg_set_buf() which needs > adjacent pages. > > Always make a copy of the signature and digest in the same buffer used to > store the key and its parameters, and pass them to sg_init_one(). Prefer it > to conditionally doing the copy if necessary, to keep the code simple. The > buffer allocated with kmalloc() is in the linear mapping area. > > Cc: stable@vger.kernel.org # 4.9.x > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > Suggested-by: Eric Biggers <ebiggers@kernel.org> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Eric Biggers <ebiggers@google.com> I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue. Tested-by: Stefan Berger <stefanb@linux.ibm.com> > --- > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > index 2f8352e88860..49a3f7c01149 100644 > --- a/crypto/asymmetric_keys/public_key.c > +++ b/crypto/asymmetric_keys/public_key.c > @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey, > struct crypto_wait cwait; > struct crypto_akcipher *tfm; > struct akcipher_request *req; > - struct scatterlist src_sg[2]; > + struct scatterlist src_sg; > char alg_name[CRYPTO_MAX_ALG_NAME]; > - char *key, *ptr; > + char *buf, *ptr; > + size_t buf_len; > int ret; > > pr_devel("==>%s()\n", __func__); > @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey, > if (!req) > goto error_free_tfm; > > - key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, > - GFP_KERNEL); > - if (!key) > + buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, > + sig->s_size + sig->digest_size); > + > + buf = kmalloc(buf_len, GFP_KERNEL); > + if (!buf) > goto error_free_req; > > - memcpy(key, pkey->key, pkey->keylen); > - ptr = key + pkey->keylen; > + memcpy(buf, pkey->key, pkey->keylen); > + ptr = buf + pkey->keylen; > ptr = pkey_pack_u32(ptr, pkey->algo); > ptr = pkey_pack_u32(ptr, pkey->paramlen); > memcpy(ptr, pkey->params, pkey->paramlen); > > if (pkey->key_is_private) > - ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen); > + ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen); > else > - ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen); > + ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen); > if (ret) > - goto error_free_key; > + goto error_free_buf; > > if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) { > ret = cert_sig_digest_update(sig, tfm); > if (ret) > - goto error_free_key; > + goto error_free_buf; > } > > - sg_init_table(src_sg, 2); > - sg_set_buf(&src_sg[0], sig->s, sig->s_size); > - sg_set_buf(&src_sg[1], sig->digest, sig->digest_size); > - akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size, > + memcpy(buf, sig->s, sig->s_size); > + memcpy(buf + sig->s_size, sig->digest, sig->digest_size); > + > + sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size); > + akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size, > sig->digest_size); > crypto_init_wait(&cwait); > akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG | > @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey, > crypto_req_done, &cwait); > ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait); > > -error_free_key: > - kfree(key); > +error_free_buf: > + kfree(buf); > error_free_req: > akcipher_request_free(req); > error_free_tfm: ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-06-01 21:00 ` Stefan Berger @ 2023-06-02 9:17 ` Roberto Sassu 2023-06-02 13:17 ` Eric Biggers 0 siblings, 1 reply; 24+ messages in thread From: Roberto Sassu @ 2023-06-02 9:17 UTC (permalink / raw) To: Stefan Berger, dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, ebiggers, Jarkko Sakkinen Cc: linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote: > > On 12/27/22 09:27, Roberto Sassu wrote: > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > mapping") checks that both the signature and the digest reside in the > > linear mapping area. > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > stack support") made it possible to move the stack in the vmalloc area, > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > adjacent pages. > > > > Always make a copy of the signature and digest in the same buffer used to > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > to conditionally doing the copy if necessary, to keep the code simple. The > > buffer allocated with kmalloc() is in the linear mapping area. > > > > Cc: stable@vger.kernel.org # 4.9.x > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue. > > Tested-by: Stefan Berger <stefanb@linux.ibm.com> Thanks, Stefan. I did multiple attempts to have the patch included, but I didn't have any luck with the maintainers (David, Jarkko). It would be awesome if any maintainer picks it. Thanks! Roberto > --- > > crypto/asymmetric_keys/public_key.c | 38 ++++++++++++++++------------- > > 1 file changed, 21 insertions(+), 17 deletions(-) > > > > diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c > > index 2f8352e88860..49a3f7c01149 100644 > > --- a/crypto/asymmetric_keys/public_key.c > > +++ b/crypto/asymmetric_keys/public_key.c > > @@ -360,9 +360,10 @@ int public_key_verify_signature(const struct public_key *pkey, > > struct crypto_wait cwait; > > struct crypto_akcipher *tfm; > > struct akcipher_request *req; > > - struct scatterlist src_sg[2]; > > + struct scatterlist src_sg; > > char alg_name[CRYPTO_MAX_ALG_NAME]; > > - char *key, *ptr; > > + char *buf, *ptr; > > + size_t buf_len; > > int ret; > > > > pr_devel("==>%s()\n", __func__); > > @@ -400,34 +401,37 @@ int public_key_verify_signature(const struct public_key *pkey, > > if (!req) > > goto error_free_tfm; > > > > - key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, > > - GFP_KERNEL); > > - if (!key) > > + buf_len = max_t(size_t, pkey->keylen + sizeof(u32) * 2 + pkey->paramlen, > > + sig->s_size + sig->digest_size); > > + > > + buf = kmalloc(buf_len, GFP_KERNEL); > > + if (!buf) > > goto error_free_req; > > > > - memcpy(key, pkey->key, pkey->keylen); > > - ptr = key + pkey->keylen; > > + memcpy(buf, pkey->key, pkey->keylen); > > + ptr = buf + pkey->keylen; > > ptr = pkey_pack_u32(ptr, pkey->algo); > > ptr = pkey_pack_u32(ptr, pkey->paramlen); > > memcpy(ptr, pkey->params, pkey->paramlen); > > > > if (pkey->key_is_private) > > - ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen); > > + ret = crypto_akcipher_set_priv_key(tfm, buf, pkey->keylen); > > else > > - ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen); > > + ret = crypto_akcipher_set_pub_key(tfm, buf, pkey->keylen); > > if (ret) > > - goto error_free_key; > > + goto error_free_buf; > > > > if (strcmp(pkey->pkey_algo, "sm2") == 0 && sig->data_size) { > > ret = cert_sig_digest_update(sig, tfm); > > if (ret) > > - goto error_free_key; > > + goto error_free_buf; > > } > > > > - sg_init_table(src_sg, 2); > > - sg_set_buf(&src_sg[0], sig->s, sig->s_size); > > - sg_set_buf(&src_sg[1], sig->digest, sig->digest_size); > > - akcipher_request_set_crypt(req, src_sg, NULL, sig->s_size, > > + memcpy(buf, sig->s, sig->s_size); > > + memcpy(buf + sig->s_size, sig->digest, sig->digest_size); > > + > > + sg_init_one(&src_sg, buf, sig->s_size + sig->digest_size); > > + akcipher_request_set_crypt(req, &src_sg, NULL, sig->s_size, > > sig->digest_size); > > crypto_init_wait(&cwait); > > akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG | > > @@ -435,8 +439,8 @@ int public_key_verify_signature(const struct public_key *pkey, > > crypto_req_done, &cwait); > > ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait); > > > > -error_free_key: > > - kfree(key); > > +error_free_buf: > > + kfree(buf); > > error_free_req: > > akcipher_request_free(req); > > error_free_tfm: ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-06-02 9:17 ` Roberto Sassu @ 2023-06-02 13:17 ` Eric Biggers 2023-06-02 13:39 ` Roberto Sassu 0 siblings, 1 reply; 24+ messages in thread From: Eric Biggers @ 2023-06-02 13:17 UTC (permalink / raw) To: Roberto Sassu Cc: Stefan Berger, dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, Jarkko Sakkinen, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Fri, Jun 02, 2023 at 11:17:04AM +0200, Roberto Sassu wrote: > On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote: > > > > On 12/27/22 09:27, Roberto Sassu wrote: > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > mapping") checks that both the signature and the digest reside in the > > > linear mapping area. > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > stack support") made it possible to move the stack in the vmalloc area, > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > adjacent pages. > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue. > > > > Tested-by: Stefan Berger <stefanb@linux.ibm.com> > > Thanks, Stefan. > > I did multiple attempts to have the patch included, but I didn't have > any luck with the maintainers (David, Jarkko). > > It would be awesome if any maintainer picks it. > > Thanks! > As the maintainers are ignoring this patch, you could try the "maintainers of last resort" (Andrew Morton or Linus Torvalds). - Eric ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() 2023-06-02 13:17 ` Eric Biggers @ 2023-06-02 13:39 ` Roberto Sassu 0 siblings, 0 replies; 24+ messages in thread From: Roberto Sassu @ 2023-06-02 13:39 UTC (permalink / raw) To: Eric Biggers Cc: Stefan Berger, dhowells, herbert, davem, zohar, dmitry.kasatkin, paul, jmorris, serge, Jarkko Sakkinen, linux-integrity, linux-security-module, keyrings, linux-crypto, linux-kernel, stable, Roberto Sassu On Fri, 2023-06-02 at 06:17 -0700, Eric Biggers wrote: > On Fri, Jun 02, 2023 at 11:17:04AM +0200, Roberto Sassu wrote: > > On Thu, 2023-06-01 at 17:00 -0400, Stefan Berger wrote: > > > On 12/27/22 09:27, Roberto Sassu wrote: > > > > From: Roberto Sassu <roberto.sassu@huawei.com> > > > > > > > > Commit ac4e97abce9b8 ("scatterlist: sg_set_buf() argument must be in linear > > > > mapping") checks that both the signature and the digest reside in the > > > > linear mapping area. > > > > > > > > However, more recently commit ba14a194a434c ("fork: Add generic vmalloced > > > > stack support") made it possible to move the stack in the vmalloc area, > > > > which is not contiguous, and thus not suitable for sg_set_buf() which needs > > > > adjacent pages. > > > > > > > > Always make a copy of the signature and digest in the same buffer used to > > > > store the key and its parameters, and pass them to sg_init_one(). Prefer it > > > > to conditionally doing the copy if necessary, to keep the code simple. The > > > > buffer allocated with kmalloc() is in the linear mapping area. > > > > > > > > Cc: stable@vger.kernel.org # 4.9.x > > > > Fixes: ba14a194a434 ("fork: Add generic vmalloced stack support") > > > > Link: https://lore.kernel.org/linux-integrity/Y4pIpxbjBdajymBJ@sol.localdomain/ > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > Reviewed-by: Eric Biggers <ebiggers@google.com> > > > > > > I just ran into an issue with OpenBMC on ARM where EVM ECDSA signature verification failed due to invalid hashes being passed to the ECDSA signature verification algorithm. This patch here resolved the issue. > > > > > > Tested-by: Stefan Berger <stefanb@linux.ibm.com> > > > > Thanks, Stefan. > > > > I did multiple attempts to have the patch included, but I didn't have > > any luck with the maintainers (David, Jarkko). > > > > It would be awesome if any maintainer picks it. > > > > Thanks! > > > > As the maintainers are ignoring this patch, you could try the "maintainers of > last resort" (Andrew Morton or Linus Torvalds). Thanks, will do. Roberto ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-06-02 13:40 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-27 14:27 [PATCH v5 0/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 2022-12-27 14:27 ` [PATCH v5 1/2] lib/mpi: Fix buffer overrun when SG is too long Roberto Sassu 2022-12-29 22:38 ` Eric Biggers 2022-12-30 13:35 ` David Laight 2022-12-30 15:39 ` Herbert Xu 2022-12-31 13:23 ` David Laight 2023-01-06 15:18 ` Herbert Xu 2023-01-16 8:57 ` Roberto Sassu 2023-01-16 9:06 ` Herbert Xu 2023-01-20 10:23 ` Roberto Sassu 2022-12-27 14:27 ` [PATCH v5 2/2] KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() Roberto Sassu 2022-12-29 22:39 ` Eric Biggers 2023-01-27 8:27 ` Roberto Sassu 2023-02-09 10:49 ` Roberto Sassu 2023-02-09 18:53 ` Eric Biggers 2023-02-09 22:46 ` Paul Moore 2023-02-10 3:56 ` Jarkko Sakkinen 2023-02-10 17:32 ` Paul Moore 2023-02-10 3:52 ` Jarkko Sakkinen 2023-05-26 7:35 ` Roberto Sassu 2023-06-01 21:00 ` Stefan Berger 2023-06-02 9:17 ` Roberto Sassu 2023-06-02 13:17 ` Eric Biggers 2023-06-02 13:39 ` Roberto Sassu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).