linux-kernel - Re: [v2] rsi: Remove stack VLA usage

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]

Date:   Tue, 27 Mar 2018 08:04:53 +0000 (UTC)
From:   Kalle Valo <kvalo@...eaurora.org>
To:     "Tobin C. Harding" <me@...in.cc>
Cc:     "Tobin C. Harding" <me@...in.cc>,
        kernel-hardening@...ts.openwall.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Tycho Andersen <tycho@...ho.ws>,
        Kees Cook <keescook@...omium.org>,
        Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [v2] rsi: Remove stack VLA usage

"Tobin C. Harding" <me@...in.cc> wrote:

> The use of stack Variable Length Arrays needs to be avoided, as they
> can be a vector for stack exhaustion, which can be both a runtime bug
> (kernel Oops) or a security flaw (overwriting memory beyond the
> stack). Also, in general, as code evolves it is easy to lose track of
> how big a VLA can get. Thus, we can end up having runtime failures
> that are hard to debug. As part of the directive[1] to remove all VLAs
> from the kernel, and build with -Wvla.
> 
> Currently rsi code uses a VLA based on a function argument to
> `rsi_sdio_load_data_master_write()`.  The function call chain is
> 
> Both these functions
> 
> 	rsi_sdio_reinit_device()
> 	rsi_probe()
> 
> start the call chain:
> 
> 	rsi_hal_device_init()
> 	rsi_load_fw()
> 	auto_fw_upgrade()
> 	ping_pong_write()
> 	rsi_sdio_load_data_master_write()
> 
> [Without familiarity with the code] it appears that none of the 4 locks
> 
> 	mutex
> 	rx_mutex
> 	tx_mutex
> 	tx_bus_mutex
> 
> are held when `rsi_sdio_load_data_master_write()` is called.  It is therefore
> safe to use kmalloc with GFP_KERNEL.
> 
> We can avoid using the VLA by using `kmalloc()` and free'ing the memory on all
> exit paths.
> 
> Change buffer from 'u8 array' to 'u8 *'.  Call `kmalloc()` to allocate memory for
> the buffer.  Using goto statement to call `kfree()` on all return paths.
> 
> It can be expected that this patch will result in a small increase in overhead
> due to the use of `kmalloc()` however this code is only called on initialization
> (and re-initialization) so this overhead should not degrade performance.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Tobin C. Harding <me@...in.cc>

Patch applied to wireless-drivers-next.git, thanks.

44f98a9332e4 rsi: Remove stack VLA usage

-- 
https://patchwork.kernel.org/patch/10283841/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches