linux-hardening - Re: [PATCH] fortify: Avoid shadowing previous locals

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: <202110251346.1E6F4CB6@keescook>
Date:   Mon, 25 Oct 2021 13:49:57 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Qian Cai <quic_qiancai@...cinc.com>
Cc:     linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fortify: Avoid shadowing previous locals

On Mon, Oct 25, 2021 at 04:15:28PM -0400, Qian Cai wrote:
> 
> 
> On 10/25/21 3:34 PM, Kees Cook wrote:
> > On Mon, Oct 25, 2021 at 02:37:28PM -0400, Qian Cai wrote:
> >> __compiletime_strlen macro expansion will shadow p_size and p_len local
> >> variables. Just rename those in __compiletime_strlen.
> > 
> > They don't escape their local context, though, right? i.e. I don't see a
> > problem with the existing macro. Did you encounter a specific issue that
> > this patch fixes?
> 
> Yes, this is pretty minor. There are also some extra compiling warnings (W=2)
> from it.
> 
> ./include/linux/fortify-string.h: In function 'strnlen':
> 
> ./include/linux/fortify-string.h:17:9: warning: declaration of 'p_size' shadows a previous local [-Wshadow]
> 
>    17 |  size_t p_size = __builtin_object_size(p, 1); \
> 
>       |         ^~~~~~
> 
> ./include/linux/fortify-string.h:77:17: note: in expansion of macro '__compiletime_strlen'
>    77 |  size_t p_len = __compiletime_strlen(p);
> 
>       |                 ^~~~~~~~~~~~~~~~~~~~
> 
> ./include/linux/fortify-string.h:76:9: note: shadowed declaration is here
> 
>    76 |  size_t p_size = __builtin_object_size(p, 1);
> 
>       |         ^~~~~~

Gotcha. Yeah, we have -Wshadow=local tracked here:
https://github.com/KSPP/linux/issues/152

The changes needed to fix __wait_event() are extensive, though. But yes,
there's no good reason for this macro to make things worse for W=2. ;)

I'd like to keep the existing names, so many just prefixing them with
"__" and send a v2?

Thanks!

-- 
Kees Cook