linux-kernel - Re: [PATCH] mwifiex: drop most magic numbers from mwifiex_process_tdls_action

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20200126113722.D8991C4479C@smtp.codeaurora.org>
Date:   Sun, 26 Jan 2020 11:37:22 +0000 (UTC)
From:   Kalle Valo <kvalo@...eaurora.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     linux-wireless@...r.kernel.org, <linux-kernel@...r.kernel.org>,
        Ganapathi Bhat <ganapathi.bhat@....com>,
        Nishant Sarmukadam <nishants@...vell.com>,
        Amitkumar Karwar <amitkarwar@...il.com>,
        Xinming Hu <huxinming820@...il.com>, dan.carpenter@...cle.com,
        solar@...nwall.com, wangqize888888888@...il.com,
        Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH] mwifiex: drop most magic numbers from
 mwifiex_process_tdls_action_frame()

Brian Norris <briannorris@...omium.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@...omium.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