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

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

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.

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.

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

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.

index 7869bb40..208df7c 100644
@@ -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);
 }

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

Thanks,
Ard.

Applied.

johannes