linux-kernel - Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Y2KhCyofvfG5W5hE@zn.tnic>
Date:   Wed, 2 Nov 2022 17:55:39 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     YingChi Long <me@...lyc.cn>
Cc:     chang.seok.bae@...el.com, dave.hansen@...ux.intel.com,
        david.laight@...lab.com, hpa@...or.com,
        linux-kernel@...r.kernel.org, mingo@...hat.com,
        ndesaulniers@...gle.com, pbonzini@...hat.com, tglx@...utronix.de,
        x86@...nel.org
Subject: Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

On Sat, Oct 29, 2022 at 08:25:52PM +0800, YingChi Long wrote:
> WG14 N2350 made very clear that it is an UB having type definitions
> with in "offsetof".

Pls put the working group URL note here, not below in a Link tag.

Also, write out "UB" pls.

> This patch change the implementation of macro

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

But you don't change the implementation of TYPE_ALIGN - you replace it
with _Alignof().

> I've grepped all source files to find any type definitions within
> "offsetof".
>
>     offsetof\(struct .*\{ .*,
>
> This implementation of macro "TYPE_ALIGN" seemes to be the only case
> of type definitions within offsetof in the kernel codebase.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.

What does that paragraph have to do with fixing the kernel?

Is this patch going to be part of clang? If so, which version?

Does gcc need it too?

If so, should a gcc bug be opened too?

> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
> 
> The macro TYPE_ALIGN we're replacing has behavior that matches

Who's "we"?

> _Alignof rather than __alignof__.
> 
> Signed-off-by: YingChi Long <me@...lyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT

Put that link in the text above where you talk about the *align*
differences.

> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

Aha, that's the clang patch. Put it in the text above too pls.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette