This is a cleanup of Coverity ressource leak findings for the quick & dirty
crypto testing module crypto/tcrypt.c.
All 3 changesets address function test_aead_speed() that was introduced in
53f52d7a (crypto: tcrypt - Added speed tests for AEAD crypto alogrithms in
tcrypt test suite)
The series applies against v3.15-rc2 as well as branch master in tree
git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git and is
compile tested only.
Christian Engelmayer (3):
crypto: Fix potential leak in test_aead_speed() if aad_size is too big
crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails
crypto: Fix leak of struct aead_request in test_aead_speed()
crypto/tcrypt.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
--
1.9.1
Fix a potential memory leak in the error handling of test_aead_speed(). In case
the size check on the associate data length parameter fails, the function goes
through the wrong exit label. Reported by Coverity - CID 1163870.
Signed-off-by: Christian Engelmayer <[email protected]>
---
crypto/tcrypt.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 870be7b..1856d7f 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
unsigned int *b_size;
unsigned int iv_len;
+ if (aad_size >= PAGE_SIZE) {
+ pr_err("associate data length (%u) too big\n", aad_size);
+ return;
+ }
+
if (enc == ENCRYPT)
e = "encryption";
else
@@ -323,14 +328,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
b_size = aead_sizes;
do {
assoc = axbuf[0];
-
- if (aad_size < PAGE_SIZE)
- memset(assoc, 0xff, aad_size);
- else {
- pr_err("associate data length (%u) too big\n",
- aad_size);
- goto out_nosg;
- }
+ memset(assoc, 0xff, aad_size);
sg_init_one(&asg[0], assoc, aad_size);
if ((*keysize + *b_size) > TVMEMSIZE * PAGE_SIZE) {
--
1.9.1
Fix a potential memory leak in the error handling of test_aead_speed(). In case
crypto_alloc_aead() fails, the function returns without going through the
centralized cleanup path. Reported by Coverity - CID 1163870.
Signed-off-by: Christian Engelmayer <[email protected]>
---
crypto/tcrypt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1856d7f..1849155 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -313,7 +313,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
if (IS_ERR(tfm)) {
pr_err("alg: aead: Failed to load transform for %s: %ld\n", algo,
PTR_ERR(tfm));
- return;
+ goto out_notfm;
}
req = aead_request_alloc(tfm, GFP_KERNEL);
@@ -391,6 +391,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
out:
crypto_free_aead(tfm);
+out_notfm:
kfree(sg);
out_nosg:
testmgr_free_buf(xoutbuf);
--
1.9.1
Fix leakage of memory for struct aead_request that is allocated via
aead_request_alloc() but not released via aead_request_free().
Reported by Coverity - CID 1163869.
Signed-off-by: Christian Engelmayer <[email protected]>
---
crypto/tcrypt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1849155..09c93ff2 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -320,7 +320,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
if (!req) {
pr_err("alg: aead: Failed to allocate request for %s\n",
algo);
- goto out;
+ goto out_noreq;
}
i = 0;
@@ -390,6 +390,8 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
} while (*keysize);
out:
+ aead_request_free(req);
+out_noreq:
crypto_free_aead(tfm);
out_notfm:
kfree(sg);
--
1.9.1
On Monday, April 21, 2014 at 08:47:05 PM, Christian Engelmayer wrote:
> Fix leakage of memory for struct aead_request that is allocated via
> aead_request_alloc() but not released via aead_request_free().
> Reported by Coverity - CID 1163869.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
Reviewed-by: Marek Vasut <[email protected]>
Best regards,
Marek Vasut
On Monday, April 21, 2014 at 08:46:40 PM, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In
> case crypto_alloc_aead() fails, the function returns without going through
> the centralized cleanup path. Reported by Coverity - CID 1163870.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
Looks OK to me, thanks.
Reviewed-by: Marek Vasut <[email protected]>
Best regards,
Marek Vasut
On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In
> case the size check on the associate data length parameter fails, the
> function goes through the wrong exit label. Reported by Coverity - CID
> 1163870.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
> ---
> crypto/tcrypt.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 870be7b..1856d7f 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc,
> unsigned int sec, unsigned int *b_size;
> unsigned int iv_len;
>
> + if (aad_size >= PAGE_SIZE) {
On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient here?
Cheers!
On Mon, 2014-04-21 at 20:45 +0200, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In case
> the size check on the associate data length parameter fails, the function goes
> through the wrong exit label. Reported by Coverity - CID 1163870.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
Acked.
Tim
> ---
> crypto/tcrypt.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 870be7b..1856d7f 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
> unsigned int *b_size;
> unsigned int iv_len;
>
> + if (aad_size >= PAGE_SIZE) {
> + pr_err("associate data length (%u) too big\n", aad_size);
> + return;
> + }
> +
> if (enc == ENCRYPT)
> e = "encryption";
> else
> @@ -323,14 +328,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
> b_size = aead_sizes;
> do {
> assoc = axbuf[0];
> -
> - if (aad_size < PAGE_SIZE)
> - memset(assoc, 0xff, aad_size);
> - else {
> - pr_err("associate data length (%u) too big\n",
> - aad_size);
> - goto out_nosg;
> - }
> + memset(assoc, 0xff, aad_size);
> sg_init_one(&asg[0], assoc, aad_size);
>
> if ((*keysize + *b_size) > TVMEMSIZE * PAGE_SIZE) {
On Mon, 2014-04-21 at 20:46 +0200, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In case
> crypto_alloc_aead() fails, the function returns without going through the
> centralized cleanup path. Reported by Coverity - CID 1163870.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
Acked.
Tim
> ---
> crypto/tcrypt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 1856d7f..1849155 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -313,7 +313,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
> if (IS_ERR(tfm)) {
> pr_err("alg: aead: Failed to load transform for %s: %ld\n", algo,
> PTR_ERR(tfm));
> - return;
> + goto out_notfm;
> }
>
> req = aead_request_alloc(tfm, GFP_KERNEL);
> @@ -391,6 +391,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>
> out:
> crypto_free_aead(tfm);
> +out_notfm:
> kfree(sg);
> out_nosg:
> testmgr_free_buf(xoutbuf);
On Mon, 2014-04-21 at 20:47 +0200, Christian Engelmayer wrote:
> Fix leakage of memory for struct aead_request that is allocated via
> aead_request_alloc() but not released via aead_request_free().
> Reported by Coverity - CID 1163869.
>
> Signed-off-by: Christian Engelmayer <[email protected]>
Acked. Thanks for fixing the leak.
Tim
> ---
> crypto/tcrypt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 1849155..09c93ff2 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -320,7 +320,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
> if (!req) {
> pr_err("alg: aead: Failed to allocate request for %s\n",
> algo);
> - goto out;
> + goto out_noreq;
> }
>
> i = 0;
> @@ -390,6 +390,8 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
> } while (*keysize);
>
> out:
> + aead_request_free(req);
> +out_noreq:
> crypto_free_aead(tfm);
> out_notfm:
> kfree(sg);
On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut <[email protected]> wrote:
> On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> > + if (aad_size >= PAGE_SIZE) {
>
> On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient here?
From what I have seen how the buffers are allocated via __get_free_page() I
thought so too. However, as it previously read
if (aad_size < PAGE_SIZE)
memset(assoc, 0xff, aad_size);
else {
my intention was simply to make the modification so that the bug is addressed
without introducing an additional change.
Regards,
Christian
On Wednesday, April 23, 2014 at 07:43:35 PM, Christian Engelmayer wrote:
> On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut <[email protected]> wrote:
> > On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> > > + if (aad_size >= PAGE_SIZE) {
> >
> > On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient
> > here?
>
> From what I have seen how the buffers are allocated via __get_free_page() I
> thought so too. However, as it previously read
>
> if (aad_size < PAGE_SIZE)
> memset(assoc, 0xff, aad_size);
> else {
>
> my intention was simply to make the modification so that the bug is
> addressed without introducing an additional change.
I fully agree with you. I was just curious about the comparison here.
Best regards,
Marek Vasut
On Mon, Apr 21, 2014 at 08:44:39PM +0200, Christian Engelmayer wrote:
> This is a cleanup of Coverity ressource leak findings for the quick & dirty
> crypto testing module crypto/tcrypt.c.
>
> All 3 changesets address function test_aead_speed() that was introduced in
> 53f52d7a (crypto: tcrypt - Added speed tests for AEAD crypto alogrithms in
> tcrypt test suite)
All applied. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt