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
Quoted 10 lines · Dan Carpenter
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.c61 struct crypto_aead *ieee80211_aes_gmac_key_setup(const u8 key[], 71 err = crypto_aead_setkey(tfm, key, key_len); 72 if (!err) ^^^^
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
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.
Signature · 1 line
Jouni Malinen PGP id EFC895FA
PATCH 1/1 mac80111: aes_ccm: cleanup ieee80211_aes_key_setup_encrypt()
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.
net/mac80211/aes_ccm.c +8 -4
@@ -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()
Quoted 6 lines · 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 <[email protected]>
Thanks,
Ard.
Quoted 25 lines
net/mac80211/aes_ccm.c +8
-4@@ -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()
In reply to Dan CarpenterQuoted 4 lines · 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.
Applied.
johannes
Quoted 5 lines · Jouni Malinen
Quoted 3 lines
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