linux-kernel - Re: [PATCH 1/2] carl9170: re-fix fortified-memset warning

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: <eba3d9ce-389b-b4cf-1af2-6a5ee9ca5049@kernel.org>
Date:   Mon, 26 Jun 2023 08:51:39 +0200
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Christian Lamparter <chunkeey@...il.com>,
        Arnd Bergmann <arnd@...db.de>, Arnd Bergmann <arnd@...nel.org>,
        Kalle Valo <kvalo@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Johannes Berg <johannes.berg@...el.com>,
        Shiji Yang <yangshiji66@...look.com>,
        Nick Kossifidis <mickflemm@...il.com>,
        Christian Marangi <ansuelsmth@...il.com>
Cc:     linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] carl9170: re-fix fortified-memset warning

On 23. 06. 23, 19:15, Christian Lamparter wrote:
> On 6/23/23 18:05, Arnd Bergmann wrote:
>> On Fri, Jun 23, 2023, at 17:38, Christian Lamparter wrote:
>>> On 6/23/23 17:23, Arnd Bergmann wrote:
>>>
>>> Wait! I want to point out this funny thing is happening in ath too!
>>>
>>> https://lore.kernel.org/linux-wireless/TYAP286MB03154F9AAFD4C35BEEDE4A99BC4CA@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/T/#mf1b8919a000fe661803c17073f48b3c410888541
>>>
>>> And that patch got NACK by Jiri Slaby because like me he suspects that
>>> this is a compiler bug.
>>
>> FWIW, that is one I don't see with clang-17 or gcc-13. The one I'm 
>> addressing
>> here is the only thing I see in ath wireless with the default set of
>> warning options, though this driver does have a couple of others that
>> are unrelated, when you enable the source data check in memcpy() by
>> building with W=1.
>>
>>   In file included from  drivers/net/wireless/ath/ath9k/xmit.c:17:
>> In file included from  include/linux/dma-mapping.h:7:
>> In file included from include/linux/string.h:254:
>> /home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call 
>> to '__read_overflow2_field' declared with 'warning' attribute: 
>> detected read beyond size of field (2nd parameter); maybe use 
>> struct_group()? [-Werror,-Wattribute-warning]
>>                          __read_overflow2_field(q_size_field, size);
>>                          ^
>> include/linux/fortify-string.h:592:4: error: call to 
>> '__read_overflow2_field' declared with 'warning' attribute: detected 
>> read beyond size of field (2nd parameter); maybe use struct_group()? 
>> [-Werror,-Wattribute-warning]
>> 2 errors generated.
>> /home/arnd/arm-soc/include/linux/fortify-string.h:592:4: error: call 
>> to '__read_overflow2_field' declared with 'warning' attribute: 
>> detected read beyond size of field (2nd parameter); maybe use 
>> struct_group()? [-Werror,-Wattribute-warning]
>>                          __read_overflow2_field(q_size_field, size);
>>
>>> so, what's going wrong with fortified there?
>>
>> Kees might have a better answer to that, my best guess is that
>> the one I'm addressing stems from the confusion between different
>> union members.
>>
>> Doing the randconfig builds with the latest compilers, carl9170 is the
>> only one I see with fortified-string warnings, and there are a few
>> dozen other drivers that I see with W=1, including one that affects
>> all wireless drivers.
> 
> Hm, question here (to Jiri as well). Do you think that a workaround patch
> for these 
> sort-of-obvious-but-compiler-bug-but-failed-to-make-a-simple-reproducer
> would be OK to get NACKed? In my case, I fiddled around with it and 
> replaced the
> the cc_ani memset in the following way:
> 
> |        memset(&common->cc_survey, 0, sizeof(common->cc_survey));
> |-       memset(&common->cc_ani, 0, sizeof(common->cc_ani));
> |+       common->cc_ani.cycles = common->cc_ani.rx_busy = 
> common->cc_ani.rx_frame = common->cc_ani.tx_frame = 0;

Nah, you are still changing the code for the compiler. And espectially 
this one calls for troubles later -- when cc_ani changes.

Again, work also with compiler guys, they are usually helpful. Both in 
helping to understand the issue (from the compiler POV) and provide a 
fix/workaround.

Even this carl9170 change looks very bad to me. While 
"memset_after(&txinfo->status, 0, rates);" means exactly what it does, 
those two memsets barely. It took me a while to understand what is going 
on and that it is the same. Don't do this.

Perhaps we need memset_no_check()?

thanks,
-- 
js
suse labs