re: mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers

re: mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers

[Dan Carpenter]

Hello Jouni Malinen,

The patch 8ade538bf39b: "mac80111: Add BIP-GMAC-128 and BIP-GMAC-256
ciphers" from Jan 24, 2015, leads to the following static checker
warning:

	net/mac80211/aes_gmac.c:74 ieee80211_aes_gmac_key_setup()
	warn: we tested 'err' before and it was 'true'

net/mac80211/aes_gmac.c
    61  struct crypto_aead *ieee80211_aes_gmac_key_setup(const u8 key[],
    62                                                   size_t key_len)
    63  {
    64          struct crypto_aead *tfm;
    65          int err;
    66  
    67          tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);
    68          if (IS_ERR(tfm))
    69                  return tfm;
    70  
    71          err = crypto_aead_setkey(tfm, key, key_len);
    72          if (!err)
                    ^^^^
This is success handling.  In the kernel, everyone expects error
hanlding like "if (err) " so this makes the code hard to read if you
have a job and need to read code quickly.  At first I missed the "!"
character, and then I thought,  "What??  Is err a pointer?"

    73                  return tfm;
    74          if (!err)
    75                  err = crypto_aead_setauthsize(tfm, GMAC_MIC_LEN);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is dead code.

    76  
    77          crypto_free_aead(tfm);
    78          return ERR_PTR(err);

This function should be a list of commands in a row with tiny detours
for exceptions and error handling.  It messes everyone up if the success
path is hidden somewhere in the middle and it leads to bugs like this.

    79  }

regards,
dan carpenter

Re: mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers

[Jouni Malinen]

On Thu, Mar 19, 2015 at 03:53:14PM +0300, Dan Carpenter wrote:
> The patch 8ade538bf39b: "mac80111: Add BIP-GMAC-128 and BIP-GMAC-256
> ciphers" from Jan 24, 2015, leads to the following static checker
> warning:

> net/mac80211/aes_gmac.c
>     61  struct crypto_aead *ieee80211_aes_gmac_key_setup(const u8 key[],

>     71          err = crypto_aead_setkey(tfm, key, key_len);
>     72          if (!err)
>                     ^^^^
> This is success handling.  In the kernel, everyone expects error
> hanlding like "if (err) " so this makes the code hard to read if you
> have a job and need to read code quickly.  At first I missed the "!"
> character, and then I thought,  "What??  Is err a pointer?"

This follows the style used in net/mac80211/aes_ccm.c for an equivalent
key_setup function. I have no issues in someone cleaning that up as long
as it is done consistently through all net/mac80211 key_setup functions.

>     73                  return tfm;
>     74          if (!err)
>     75                  err = crypto_aead_setauthsize(tfm, GMAC_MIC_LEN);
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is dead code.

That's interesting.. This must be some kind of rebasing issue since this
obviously makes no sense. Lines 72-73 were supposed to be after this
line 75 just like this is done in aes_gcm.c and aes_ccm.c. Thanks for
reporting this, I'll fix it.

>     77          crypto_free_aead(tfm);
>     78          return ERR_PTR(err);
> 
> This function should be a list of commands in a row with tiny detours
> for exceptions and error handling.  It messes everyone up if the success
> path is hidden somewhere in the middle and it leads to bugs like this.

I'm not sure that would be behind this specific bug, but like noted
above, no objection to someone cleaning this up consistently.

-- 
Jouni Malinen                                            PGP id EFC895FA

[patch] mac80111: aes_ccm: cleanup ieee80211_aes_key_setup_encrypt()

[Dan Carpenter]

This code is written using an anti-pattern called "success handling"
which makes it hard to read, especially if you are used to normal kernel
style.  It should instead be written as a list of directives in a row
with branches for error handling.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7869bb40..208df7c 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -85,11 +85,15 @@ struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
 		return tfm;
 
 	err = crypto_aead_setkey(tfm, key, key_len);
-	if (!err)
-		err = crypto_aead_setauthsize(tfm, mic_len);
-	if (!err)
-		return tfm;
+	if (err)
+		goto free_aead;
+	err = crypto_aead_setauthsize(tfm, mic_len);
+	if (err)
+		goto free_aead;
+
+	return tfm;
 
+free_aead:
 	crypto_free_aead(tfm);
 	return ERR_PTR(err);
 }

Re: mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers

[Dan Carpenter]

On Mon, Mar 23, 2015 at 03:24:09PM +0200, Jouni Malinen wrote:
> > This function should be a list of commands in a row with tiny detours
> > for exceptions and error handling.  It messes everyone up if the success
> > path is hidden somewhere in the middle and it leads to bugs like this.
> 
> I'm not sure that would be behind this specific bug, 

It is though.

1) The code here:

	err = crypto_aead_setkey(tfm, key, key_len);
	if (!err)
		return tfm;

   That looks like normal error handling code so your mind glosses over
   it.  Reviewers are human and this is a blind spot.

2) If you do it in normal kernel style then the return would have been
   at level 1 indent so the static checkers would have complained about
   the unreachable code.  This was caught with a static checker anyway,
   I suppose, but that static check has a lot more false positives so
   I'm not sure I can release it.

3) This code is just confusing.  The error and the success paths are
   twisted together.  After the first allocation then the exclusive
   parts of the success path move from indent level 1 to indent level 2.
   Confusing code is more likely to have bugs.

regards,
dan carpenter

Re: [patch] mac80111: aes_ccm: cleanup ieee80211_aes_key_setup_encrypt()

[Ard Biesheuvel]

On 23 March 2015 at 15:08, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> This code is written using an anti-pattern called "success handling"
> which makes it hard to read, especially if you are used to normal kernel
> style.  It should instead be written as a list of directives in a row
> with branches for error handling.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Thanks,
Ard.

>
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> index 7869bb40..208df7c 100644
> --- a/net/mac80211/aes_ccm.c
> +++ b/net/mac80211/aes_ccm.c
> @@ -85,11 +85,15 @@ struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
>                 return tfm;
>
>         err = crypto_aead_setkey(tfm, key, key_len);
> -       if (!err)
> -               err = crypto_aead_setauthsize(tfm, mic_len);
> -       if (!err)
> -               return tfm;
> +       if (err)
> +               goto free_aead;
> +       err = crypto_aead_setauthsize(tfm, mic_len);
> +       if (err)
> +               goto free_aead;
> +
> +       return tfm;
>
> +free_aead:
>         crypto_free_aead(tfm);
>         return ERR_PTR(err);
>  }

Re: [patch] mac80111: aes_ccm: cleanup ieee80211_aes_key_setup_encrypt()

[Johannes Berg]

On Mon, 2015-03-23 at 17:08 +0300, Dan Carpenter wrote:
> This code is written using an anti-pattern called "success handling"
> which makes it hard to read, especially if you are used to normal kernel
> style.  It should instead be written as a list of directives in a row
> with branches for error handling.

Applied.

johannes