linux-kernel - Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new u64 field

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]

Message-ID: <20160305135006.GA15928@gmail.com>
Date:	Sat, 5 Mar 2016 14:50:06 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Peter Anvin <hpa@...or.com>, Dave Hansen <dave@...1.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Helge Deller <deller@....de>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:mm/pkeys] mm/pkeys: Fix siginfo ABI breakage caused by new
 u64 field


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Thu, Mar 3, 2016 at 8:53 AM, tip-bot for Dave Hansen
> <tipbot@...or.com> wrote:
> >
> > If u64 has a natural alignment of 8 bytes (this is rare, most 32-bit
> > platforms align it to 4 bytes), then the leadup to the _sifields union
> > matters:
> 
> Side note: I'm not sure that "this is rare" comment is necessarily correct.
> 
> I think natural alignment is pretty common, even for 32-bit targets.
> x86-32 is I think the exception rather than the rule.
> 
> There is some real odd case iirc - embedded m68k, which has some
> ridiculous alignment rules. I think it only ever aligns to 16-bit
> boundaries.

So I got curious about this, but couldn't find any good online documentation about 
the alignment defaults of various architectures that GCC supports. So I reverted 
the fix and added the new check from linux-next:

    Revert "mm/pkeys: Fix siginfo ABI breakage caused by new u64 field"
    kernel/signal.c: add compile-time check for __ARCH_SI_PREAMBLE_SIZE

... which does:

 void __init signals_init(void)
 {
+       /* If this check fails, the __ARCH_SI_PREAMBLE_SIZE value is wrong! */
+       BUILD_BUG_ON(__ARCH_SI_PREAMBLE_SIZE
+               != offsetof(struct siginfo, _sifields._pad));
+

and tested it on the -tip cross-arch build testing suite, which gave the following 
result (only 32-bit architectures listed):

                                 (warns)               (warns)
testing     x86-32:  -git:  pass (    0),  -tip:  pass (    0)
testing        arm:  -git:  pass (    1),  -tip:  FAIL  .....
testing   blackfin:  -git:  pass (    0),  -tip:  pass (    0)
testing       cris:  -git:  pass (   32),  -tip:  pass (   32)
testing        frv:  -git:  pass (    1),  -tip:  FAIL  .....
testing       m32r:  -git:  pass (    6),  -tip:  pass (    6)
testing       m68k:  -git:  pass (    1),  -tip:  pass (    1)
testing microblaze:  -git:  pass (    0),  -tip:  pass (    0)
testing       mips:  -git:  pass (    1),  -tip:  FAIL  .....
testing   openrisc:  -git:  pass (    2),  -tip:  pass (    2)
testing     parisc:  -git:  pass (    0),  -tip:  FAIL  .....
testing         sh:  -git:  pass (   36),  -tip:  pass (   36)
testing      sparc:  -git:  pass (    0),  -tip:  FAIL  .....
testing       tile:  -git:  pass (    5),  -tip:  pass (    5)
testing     xtensa:  -git:  pass (    0),  -tip:  FAIL  .....
testing  powerpc32:  -git:  pass (    0),  -tip:  FAIL  .....

so if my test is correct then it's 9 architectures that align u64 to 4 bytes, vs. 
7 that align it to 8 bytes.

So naturally aligned u64 is definitely not 'rare' (so the characterisation in my 
changelog is wrong), but it's not dominant either.

FWIIW: if we only list 'major' architectures then x86-32 is indeed the odd one 
out...

> I do keep coming back to the fact that we should *probably* just do
> something like
> 
>     typedef unsigned long long __attribute__((aligned(8))) __u64;
> 
> and then introduce a separate "u64_unaligned" type for all the legacy
> cases that depended on 32-bit alignment.
> 
> It's horrendously nasty to test, though.

So in theory we could test most of it by comparing the disassembly of allyesconfig 
builds, but comparing disassemblies is a pretty hard to use method in practice.

A more workable method would be to have a test .c file that includes all UAPI 
structures in existence and defines a variable out of every single one, and then 
generates a list of sizeof() values or so. But even that isn't perfect: a 
structure might shift some fields forward, into a pre-existing hole, without 
changing the sizeof? We'd need a list of all field offsets in all structures to be 
really sure, and that's nasty.

Thanks,

	Ingo