From: Kalle Valo <kvalo@codeaurora.org>
To: Brian Norris <briannorris@chromium.org>
Cc: linux-wireless@vger.kernel.org, <linux-kernel@vger.kernel.org>,
Ganapathi Bhat <ganapathi.bhat@nxp.com>,
Nishant Sarmukadam <nishants@marvell.com>,
Amitkumar Karwar <amitkarwar@gmail.com>,
Xinming Hu <huxinming820@gmail.com>,
dan.carpenter@oracle.com, solar@openwall.com,
wangqize888888888@gmail.com,
Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH] mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()
Date: Sun, 26 Jan 2020 11:37:22 +0000 (UTC) [thread overview]
Message-ID: <20200126113722.D8991C4479C@smtp.codeaurora.org> (raw)
In-Reply-To: <20191206194535.150179-1-briannorris@chromium.org>
Brian Norris <briannorris@chromium.org> wrote:
> Before commit 1e58252e334d ("mwifiex: Fix heap overflow in
> mmwifiex_process_tdls_action_frame()"),
> mwifiex_process_tdls_action_frame() already had too many magic numbers.
> But this commit just added a ton more, in the name of checking for
> buffer overflows. That seems like a really bad idea.
>
> Let's make these magic numbers a little less magic, by
> (a) factoring out 'pos[1]' as 'ie_len'
> (b) using 'sizeof' on the appropriate source or destination fields where
> possible, instead of bare numbers
> (c) dropping redundant checks, per below.
>
> Regarding redundant checks: the beginning of the loop has this:
>
> if (pos + 2 + pos[1] > end)
> break;
>
> but then individual 'case's include stuff like this:
>
> if (pos > end - 3)
> return;
> if (pos[1] != 1)
> return;
>
> Note that the second 'return' (validating the length, pos[1]) combined
> with the above condition (ensuring 'pos + 2 + length' doesn't exceed
> 'end'), makes the first 'return' (whose 'if' can be reworded as 'pos >
> end - pos[1] - 2') redundant. Rather than unwind the magic numbers
> there, just drop those conditions.
>
> Fixes: 1e58252e334d ("mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
Patch applied to wireless-drivers-next.git, thanks.
70e5b8f445fd mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame()
--
https://patchwork.kernel.org/patch/11277011/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
prev parent reply other threads:[~2020-01-26 11:37 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-06 19:45 [PATCH] mwifiex: drop most magic numbers from mwifiex_process_tdls_action_frame() Brian Norris
2019-12-09 10:58 ` Kalle Valo
[not found] ` <0101016eea4fa7f5-e04b23cd-17a0-4306-8100-7761f1161da3-000000@us-west-2.amazonses.com>
2020-01-13 19:05 ` Brian Norris
2020-01-14 13:41 ` Kalle Valo
2020-01-26 11:37 ` Kalle Valo [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=20200126113722.D8991C4479C@smtp.codeaurora.org \
--to=kvalo@codeaurora.org \
--cc=amitkarwar@gmail.com \
--cc=briannorris@chromium.org \
--cc=dan.carpenter@oracle.com \
--cc=ganapathi.bhat@nxp.com \
--cc=huxinming820@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=nishants@marvell.com \
--cc=solar@openwall.com \
--cc=wangqize888888888@gmail.com \
/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).