Re: [PATCH] staging: rtl8723bs: Avoid memset() in aes_cipher() and aes_decipher()

[Dan Carpenter <dan.carpenter@linaro.org>]

On Mon, Jun 09, 2025 at 02:13:14PM -0700, Nathan Chancellor wrote:
> After commit 6f110a5e4f99 ("Disable SLUB_TINY for build testing"), which
> causes CONFIG_KASAN to be enabled in allmodconfig again, arm64
> allmodconfig builds with older versions of clang (15 through 17) show an
> instance of -Wframe-larger-than (which breaks the build with
> CONFIG_WERROR=y):
> 
>   drivers/staging/rtl8723bs/core/rtw_security.c:1287:5: error: stack frame size (2208) exceeds limit (2048) in 'rtw_aes_decrypt' [-Werror,-Wframe-larger-than]
>    1287 | u32 rtw_aes_decrypt(struct adapter *padapter, u8 *precvframe)
>         |     ^
> 
> This comes from aes_decipher() being inlined in rtw_aes_decrypt().
> Running the same build with CONFIG_FRAME_WARN=128 shows aes_cipher()
> also uses a decent amount of stack, just under the limit of 2048:
> 
>   drivers/staging/rtl8723bs/core/rtw_security.c:864:19: warning: stack frame size (1952) exceeds limit (128) in 'aes_cipher' [-Wframe-larger-than]
>     864 | static signed int aes_cipher(u8 *key, uint      hdrlen,
>         |                   ^
> 
> -Rpass-analysis=stack-frame-layout only shows one large structure on the
> stack, which is the ctx variable inlined from aes128k128d(). A good
> number of the other variables come from the additional checks of
> fortified string routines, which are present in memset(), which both
> aes_cipher() and aes_decipher() use to initialize some temporary
> buffers. In this case, since the size is known at compile time, these
> additional checks should not result in any code generation changes but
> allmodconfig has several sanitizers enabled, which may make it harder
> for the compiler to eliminate the compile time checks and the variables
> that come about from them.
> 
> The memset() calls are just initializing these buffers to zero, so use
> '= {}' instead, which is used all over the kernel and does the exact
> same thing as memset() without the fortify checks, which drops the stack
> usage of these functions by a few hundred kilobytes.
> 
>   drivers/staging/rtl8723bs/core/rtw_security.c:864:19: warning: stack frame size (1584) exceeds limit (128) in 'aes_cipher' [-Wframe-larger-than]
>     864 | static signed int aes_cipher(u8 *key, uint      hdrlen,
>         |                   ^
>   drivers/staging/rtl8723bs/core/rtw_security.c:1271:5: warning: stack frame size (1456) exceeds limit (128) in 'rtw_aes_decrypt' [-Wframe-larger-than]
>    1271 | u32 rtw_aes_decrypt(struct adapter *padapter, u8 *precvframe)
>         |     ^
> 
> Cc: stable@vger.kernel.org
> Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---

Yep.  I recently re-reviewed this because someone wrote a blog which said
that compilers were implementing it incorrectly and we need to use
memset().  However they misunderstood the rules and their tests were
flawed.  Using "= {}" is safe.

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter