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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 31 Oct 2022 11:29:54 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     YingChi Long <me@...lyc.cn>
Cc:     bp@...en8.de, 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,
        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 5:27 AM YingChi Long <me@...lyc.cn> wrote:
>
> WG14 N2350 made very clear that it is an UB having type definitions with
> in "offsetof". This patch change the implementation of macro
> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.
>
> 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.
>
> 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
> _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
> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

YingChi,
You may retain my reviewed by tag when resending a rebased patch that
hasn't changed significantly.

Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>

That reminds me, I need to resend one of my own patches; the x86
maintainers must be very busy.

> ---
> v3:
> - commit message changes suggested by Nick and David
>
> v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
> Signed-off-by: YingChi Long <me@...lyc.cn>
> ---
>  arch/x86/kernel/fpu/init.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 8946f89761cc..851eb13edc01 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
>         fpu__init_system_mxcsr();
>  }
>
> -/* Get alignment of the TYPE. */
> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> -
>  /*
>   * Enforce that 'MEMBER' is the last field of 'TYPE'.
>   *
> @@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
>   * because that's how C aligns structs.
>   */
>  #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
> -       BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
> -                                          TYPE_ALIGN(TYPE)))
> +       BUILD_BUG_ON(sizeof(TYPE) !=         \
> +                    ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))
>
>  /*
>   * We append the 'struct fpu' to the task_struct:
> --
> 2.37.4
>


-- 
Thanks,
~Nick Desaulniers