[v2] examples/fips_validation: fix memory leak in sha test

  There is wrong size used for allocation of digest buffer which in
some cases cause memory corruption. Also, fixed places where memory
leak is observed. This fix would enable sha 384 and 512 NIST vectors
be supported fully.

Fixes: 93d797d94f1 ("examples/fips_validation: add parsing for sha")

Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
---
v2:
 - doc update to support 384 and 512 sha.
---
 doc/guides/sample_app_ug/fips_validation.rst   |  2 +-
 examples/fips_validation/fips_validation_sha.c | 10 ++++++++--
 examples/fips_validation/main.c                |  1 +
 3 files changed, 10 insertions(+), 3 deletions(-)
  

On Sat, Jul 2, 2022 at 4:59 PM Gowrishankar Muthukrishnan
<gmuthukrishn@marvell.com> wrote:
>
> There is wrong size used for allocation of digest buffer which in
> some cases cause memory corruption. Also, fixed places where memory
> leak is observed. This fix would enable sha 384 and 512 NIST vectors
> be supported fully.
>
> Fixes: 93d797d94f1 ("examples/fips_validation: add parsing for sha")

This sha1 is not valid anymore, or maybe Akhil will squash the fix in
the next-crypto tree before reaching main.

>
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> ---
> v2:
>  - doc update to support 384 and 512 sha.
> ---
>  doc/guides/sample_app_ug/fips_validation.rst   |  2 +-
>  examples/fips_validation/fips_validation_sha.c | 10 ++++++++--
>  examples/fips_validation/main.c                |  1 +
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/sample_app_ug/fips_validation.rst b/doc/guides/sample_app_ug/fips_validation.rst
> index 4b68226665..6f4bd34726 100644
> --- a/doc/guides/sample_app_ug/fips_validation.rst
> +++ b/doc/guides/sample_app_ug/fips_validation.rst
> @@ -63,7 +63,7 @@ ACVP
>      * AES-CMAC (128,192,256) - AFT
>      * AES-XTS (128,256) - AFT
>      * HMAC (SHA1, SHA224, SHA256, SHA384, SHA512)
> -    * SHA (1,256) - AFT, MCT
> +    * SHA (1, 256, 384, 512) - AFT, MCT
>
>
>  Application Information
> diff --git a/examples/fips_validation/fips_validation_sha.c b/examples/fips_validation/fips_validation_sha.c
> index a2928618d7..538cb6647a 100644
> --- a/examples/fips_validation/fips_validation_sha.c
> +++ b/examples/fips_validation/fips_validation_sha.c
> @@ -229,13 +229,19 @@ parse_test_sha_json_algorithm(void)
>         for (i = 0; i < RTE_DIM(phsc); i++) {
>                 if (info.interim_info.sha_data.algo == phsc[i].algo) {
>                         vec.cipher_auth.digest.len = atoi(phsc[i].str);
> -                       vec.cipher_auth.digest.val = calloc(0, vec.cipher_auth.digest.len * 8);
> +                       if (vec.cipher_auth.digest.val)
> +                               free(vec.cipher_auth.digest.val);

Unneeded if().

We just did a tree-wide cleanup to avoid these.
Please don't reintroduce some.


> +
> +                       vec.cipher_auth.digest.val = calloc(1, vec.cipher_auth.digest.len);

Don't we need to check for allocation success?


>                         break;
>                 }
>         }
>
> -       if (i == RTE_DIM(phsc))
> +       if (i == RTE_DIM(phsc)) {
> +               free(vec.cipher_auth.digest.val);
> +               vec.cipher_auth.digest.val = NULL;
>                 return -1;
> +       }
>
>         return 0;
>  }
  

> On Sat, Jul 2, 2022 at 4:59 PM Gowrishankar Muthukrishnan
> <gmuthukrishn@marvell.com> wrote:
> >
> > There is wrong size used for allocation of digest buffer which in
> > some cases cause memory corruption. Also, fixed places where memory
> > leak is observed. This fix would enable sha 384 and 512 NIST vectors
> > be supported fully.
> >
> > Fixes: 93d797d94f1 ("examples/fips_validation: add parsing for sha")
> 
> This sha1 is not valid anymore, or maybe Akhil will squash the fix in
> the next-crypto tree before reaching main.
Fixed the SHA and applied on top of next-crypto as I had already asked Thomas to pull the tree.
  

> vec.cipher_auth.digest.len * 8);
> > +                       if (vec.cipher_auth.digest.val)
> > +                               free(vec.cipher_auth.digest.val);
> 
> Unneeded if().
> 
Ack.

> We just did a tree-wide cleanup to avoid these.
> Please don't reintroduce some.
> 
> 
> > +
> > +                       vec.cipher_auth.digest.val = calloc(1,
> > + vec.cipher_auth.digest.len);
> 
> Don't we need to check for allocation success?
> 
Sure. I ll post fix for the  above and thanks for the review.

Regards,
Gowrishankar
> 
> >                         break;
> >                 }
> >         }
> >
> > -       if (i == RTE_DIM(phsc))
> > +       if (i == RTE_DIM(phsc)) {
> > +               free(vec.cipher_auth.digest.val);
> > +               vec.cipher_auth.digest.val = NULL;
> >                 return -1;
> > +       }
> >
> >         return 0;
> >  }
> 
> 
> --
> David Marchand
  

04/07/2022 09:50, Akhil Goyal:
> > On Sat, Jul 2, 2022 at 4:59 PM Gowrishankar Muthukrishnan
> > <gmuthukrishn@marvell.com> wrote:
> > >
> > > There is wrong size used for allocation of digest buffer which in
> > > some cases cause memory corruption. Also, fixed places where memory
> > > leak is observed. This fix would enable sha 384 and 512 NIST vectors
> > > be supported fully.
> > >
> > > Fixes: 93d797d94f1 ("examples/fips_validation: add parsing for sha")
> > 
> > This sha1 is not valid anymore, or maybe Akhil will squash the fix in
> > the next-crypto tree before reaching main.
> Fixed the SHA and applied on top of next-crypto as I had already asked Thomas to pull the tree.

David did some good comments, I will wait for a better version of the patch.