| Message ID | 20180917172432.26733-1-dan@kernelim.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: fix a memory leak in rsa-kcs1pad's encryption mode | expand |
On 9/17/18 10:24 AM, Dan Aloni wrote: > The encryption mode of pkcs1pad never uses out_sg and out_buf, so > there's no need to allocate the buffer, which presently is not even > being freed. It is used and freed in pkcs1pad_decrypt_complete().
On Mon, Sep 17, 2018 at 12:52:44PM -0700, Tadeusz Struk wrote: > On 9/17/18 10:24 AM, Dan Aloni wrote: > > The encryption mode of pkcs1pad never uses out_sg and out_buf, so > > there's no need to allocate the buffer, which presently is not even > > being freed. > > It is used and freed in pkcs1pad_decrypt_complete(). True, but how is pkcs1pad_decrypt_complete() reachable from the encryption path of the code? Or, is there a hidden API assumption that the alg.decrypt callback will be called for every alg.encrypt call? It does not seem right. Same question for pkcs1pad_verify_complete(), which is the only other free path for this field.
On 9/17/18 1:28 PM, Dan Aloni wrote: > On Mon, Sep 17, 2018 at 12:52:44PM -0700, Tadeusz Struk wrote: >> On 9/17/18 10:24 AM, Dan Aloni wrote: >>> The encryption mode of pkcs1pad never uses out_sg and out_buf, so >>> there's no need to allocate the buffer, which presently is not even >>> being freed. >> >> It is used and freed in pkcs1pad_decrypt_complete(). > > True, but how is pkcs1pad_decrypt_complete() reachable from the > encryption path of the code? Or, is there a hidden API assumption that > the alg.decrypt callback will be called for every alg.encrypt call? It > does not seem right. Same question for pkcs1pad_verify_complete(), which > is the only other free path for this field. This is only used in the decrypt operation. The pkcs1pad_decrypt() sets the callback pkcs1pad_decrypt_complete_cb(), and calls crypto_akcipher_decrypt(). If the implementation is synchronous it returns SUCCESS and the pkcs1pad_decrypt_complete() is called directly. If the implementation is asynchronous it returns -EINPROGRESS and after the job is done the callback is called and the pkcs1pad_decrypt_complete() is called from the callback. The same pattern applies to verify.
On Mon, Sep 17, 2018 at 02:39:46PM -0700, Tadeusz Struk wrote: > On 9/17/18 1:28 PM, Dan Aloni wrote: > > On Mon, Sep 17, 2018 at 12:52:44PM -0700, Tadeusz Struk wrote: > >> On 9/17/18 10:24 AM, Dan Aloni wrote: > >>> The encryption mode of pkcs1pad never uses out_sg and out_buf, so > >>> there's no need to allocate the buffer, which presently is not even > >>> being freed. > >> > >> It is used and freed in pkcs1pad_decrypt_complete(). > > > > True, but how is pkcs1pad_decrypt_complete() reachable from the > > encryption path of the code? Or, is there a hidden API assumption that > > the alg.decrypt callback will be called for every alg.encrypt call? It > > does not seem right. Same question for pkcs1pad_verify_complete(), which > > is the only other free path for this field. > > This is only used in the decrypt operation. The pkcs1pad_decrypt() > sets the callback pkcs1pad_decrypt_complete_cb(), and calls > crypto_akcipher_decrypt(). > If the implementation is synchronous it returns SUCCESS > and the pkcs1pad_decrypt_complete() is called directly. > If the implementation is asynchronous it returns -EINPROGRESS and > after the job is done the callback is called and the > pkcs1pad_decrypt_complete() is called from the callback. > The same pattern applies to verify. That's also true, but what I still don't understand is how pkcs1pad_decrypt_complete() would be called when a higher layer calls to *encrypt* in roughly this API call sequence: ak_tfm = crypto_alloc_akcipher("pkcs1pad(rsa,sha256)", 0, 0); ... req = akcipher_request_alloc(ak_tfm, GFP_KERNEL); ... crypto_init_wait(&wait); ... crypto_wait_req(crypto_akcipher_encrypt(req), &wait); ... crypto_free_akcipher(ak_tfm); - under which pkcs1pad_encrypt() will be called via alg->encrypt and will allocate. Or if this API call sequence is wrong, it would be good to know in what way.
On 9/17/18 3:04 PM, Dan Aloni wrote: > That's also true, but what I still don't understand is how > pkcs1pad_decrypt_complete() would be called when a higher layer calls to > *encrypt* in roughly this API call sequence: > > ak_tfm = crypto_alloc_akcipher("pkcs1pad(rsa,sha256)", 0, 0); > ... > req = akcipher_request_alloc(ak_tfm, GFP_KERNEL); > ... > crypto_init_wait(&wait); > ... > crypto_wait_req(crypto_akcipher_encrypt(req), &wait); > ... > crypto_free_akcipher(ak_tfm); > > - under which pkcs1pad_encrypt() will be called via alg->encrypt > and will allocate. Or if this API call sequence is wrong, it would be > good to know in what way. I think you are right. I misread it and confused it with the decrypt path. The allocation of the out_buf has been removed from the pkcs1pad_sign() and it looks like the encrypt is a left over. Thanks,
On Mon, Sep 17, 2018 at 08:24:32PM +0300, Dan Aloni wrote: > The encryption mode of pkcs1pad never uses out_sg and out_buf, so > there's no need to allocate the buffer, which presently is not even > being freed. > > CC: Herbert Xu <herbert@gondor.apana.org.au> > CC: linux-crypto@vger.kernel.org > CC: "David S. Miller" <davem@davemloft.net> > Signed-off-by: Dan Aloni <dan@kernelim.com> Patch applied. Thanks.
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c index 9893dbfc1af4..812476e46821 100644 --- a/crypto/rsa-pkcs1pad.c +++ b/crypto/rsa-pkcs1pad.c @@ -261,15 +261,6 @@ static int pkcs1pad_encrypt(struct akcipher_request *req) pkcs1pad_sg_set_buf(req_ctx->in_sg, req_ctx->in_buf, ctx->key_size - 1 - req->src_len, req->src); - req_ctx->out_buf = kmalloc(ctx->key_size, GFP_KERNEL); - if (!req_ctx->out_buf) { - kfree(req_ctx->in_buf); - return -ENOMEM; - } - - pkcs1pad_sg_set_buf(req_ctx->out_sg, req_ctx->out_buf, - ctx->key_size, NULL); - akcipher_request_set_tfm(&req_ctx->child_req, ctx->child); akcipher_request_set_callback(&req_ctx->child_req, req->base.flags, pkcs1pad_encrypt_sign_complete_cb, req);
The encryption mode of pkcs1pad never uses out_sg and out_buf, so there's no need to allocate the buffer, which presently is not even being freed. CC: Herbert Xu <herbert@gondor.apana.org.au> CC: linux-crypto@vger.kernel.org CC: "David S. Miller" <davem@davemloft.net> Signed-off-by: Dan Aloni <dan@kernelim.com> --- crypto/rsa-pkcs1pad.c | 9 --------- 1 file changed, 9 deletions(-)