From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jouni Malinen <jouni@qca.qualcomm.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers
Date: Mon, 23 Mar 2015 17:40:32 +0300 [thread overview]
Message-ID: <20150323144032.GK10964@mwanda> (raw)
In-Reply-To: <20150323132409.GA20999@jouni.qca.qualcomm.com>
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
prev parent reply other threads:[~2015-03-23 14:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-19 12:53 mac80111: Add BIP-GMAC-128 and BIP-GMAC-256 ciphers Dan Carpenter
2015-03-23 13:24 ` Jouni Malinen
2015-03-23 14:08 ` [patch] mac80111: aes_ccm: cleanup ieee80211_aes_key_setup_encrypt() Dan Carpenter
2015-03-24 9:32 ` Ard Biesheuvel
2015-03-30 8:40 ` Johannes Berg
2015-03-23 14:40 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150323144032.GK10964@mwanda \
--to=dan.carpenter@oracle.com \
--cc=jouni@qca.qualcomm.com \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).