[PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Dave Hansen]

This responds to the feedback from Ingo that we should be using
explicitly-sized types and fixes a typo in the patch description.

--

From: Dave Hansen <dave.hansen@linux.intel.com>

Stephen Rothwell reported:

	http://lkml.kernel.org/r/20160226164406.065a1ffc@canb.auug.org.au

that the Memory Protection Keys patches from the tip tree broke
a build-time check on an ARM build because they changed the ABI
of siginfo.

A u64 was used for the protection key field in siginfo.  When the
containing union was aligned, this u64 unioned nicely with the
two 'void *'s in _addr_bnd.  But, on 32-bit, if the union was
unaligned, the u64 might grow the size of the union, breaking the
ABI for subsequent fields.

To fix this, we replace the u64 with an '__u32'.  The __u32 is
guaranteed to union well with the pointers from _addr_bnd.  It is
also plenty large enough to store the 16-bit pkey we have today
on x86.

I also shouldn't have been using a u64 in a userspace API to begin
with.

Fixes: cd0ea35ff551 ("signals, pkeys: Notify userspace about protection key faults")
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Acked-by: Stehen Rothwell <sfr@canb.auug.org.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-next@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Helge Deller <deller@gmx.de>
---

 b/arch/ia64/include/uapi/asm/siginfo.h |    2 +-
 b/arch/mips/include/uapi/asm/siginfo.h |    2 +-
 b/include/uapi/asm-generic/siginfo.h   |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff -puN include/uapi/asm-generic/siginfo.h~pkeys-101-fix-siginfo include/uapi/asm-generic/siginfo.h
--- a/include/uapi/asm-generic/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.327228965 -0800
+++ b/include/uapi/asm-generic/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -98,7 +98,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff -puN arch/mips/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo arch/mips/include/uapi/asm/siginfo.h
--- a/arch/mips/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.330229103 -0800
+++ b/arch/mips/include/uapi/asm/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -93,7 +93,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
diff -puN arch/ia64/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo arch/ia64/include/uapi/asm/siginfo.h
--- a/arch/ia64/include/uapi/asm/siginfo.h~pkeys-101-fix-siginfo	2016-02-29 09:22:45.331229149 -0800
+++ b/arch/ia64/include/uapi/asm/siginfo.h	2016-02-29 09:22:45.333229241 -0800
@@ -70,7 +70,7 @@ typedef struct siginfo {
 					void __user *_upper;
 				} _addr_bnd;
 				/* used when si_code=SEGV_PKUERR */
-				u64 _pkey;
+				__u32 _pkey;
 			};
 		} _sigfault;
 
_

Re: [PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Stephen Rothwell]

Hi Dave,

On Mon, 29 Feb 2016 14:17:33 -0800 Dave Hansen <dave@sr71.net> wrote:
>
> To fix this, we replace the u64 with an '__u32'.  The __u32 is
> guaranteed to union well with the pointers from _addr_bnd.  It is
> also plenty large enough to store the 16-bit pkey we have today
> on x86.

That looks better, thanks.

-- 
Cheers,
Stephen Rothwell

Re: [PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Ingo Molnar]

* Dave Hansen <dave@sr71.net> wrote:

> 
> This responds to the feedback from Ingo that we should be using
> explicitly-sized types and fixes a typo in the patch description.
> 
> --
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> Stephen Rothwell reported:
> 
> 	http://lkml.kernel.org/r/20160226164406.065a1ffc@canb.auug.org.au
> 
> that the Memory Protection Keys patches from the tip tree broke
> a build-time check on an ARM build because they changed the ABI
> of siginfo.

Ok, so the reason we didn't see that build failure is because the generic 
kernel/signal.c build time check for these types of bugs is in -mm, not yet 
upstream.

> A u64 was used for the protection key field in siginfo.  When the
> containing union was aligned, this u64 unioned nicely with the
> two 'void *'s in _addr_bnd.  But, on 32-bit, if the union was
> unaligned, the u64 might grow the size of the union, breaking the
> ABI for subsequent fields.
>
> To fix this, we replace the u64 with an '__u32'.  The __u32 is
> guaranteed to union well with the pointers from _addr_bnd.  It is
> also plenty large enough to store the 16-bit pkey we have today
> on x86.
> 
> I also shouldn't have been using a u64 in a userspace API to begin
> with.

Well, it's __u64 that we use in UAPIs, and they can be used just fine, as long as 
the structure's field alignments is managed explicitly, i.e. there's no automatic 
alignment padding done by the compiler.

I think we should add a warning (to x86 at least) if the packed siginfo size is 
the same as the unpacked one.

This could be done with a bit of preprocessor trickery, I think the following 
would work, in a standalone .c file:

#define __ARCH_SI_ATTRIBUTES __attribute__((aligned(8))) __packed
#include <asm-generic/siginfo.h>

int siginfo_size_packed = sizeof(struct siginfo);

and another .c file could calculate the regular size:

#include <asm/siginfo.h>

int siginfo_size = sizeof(struct siginfo);

and then a (host side) build time check could link those two .c's, run that and 
compare the two values.

or something like that.

This checking mechanism could then be extended to other user ABI definitions as 
well, such as include/uapi/linux/perf_event.h.

Thanks,

	Ingo

Re: [PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> > I also shouldn't have been using a u64 in a userspace API to begin with.
> 
> Well, it's __u64 that we use in UAPIs, and they can be used just fine, as long 
> as the structure's field alignments is managed explicitly, i.e. there's no 
> automatic alignment padding done by the compiler.

Btw., what we should not have used in a modern user ABI are variable size 
pointers:

                                struct {
                                        void __user *_lower;
                                        void __user *_upper;
                                } _addr_bnd;

we should have used constant size structure elements for that, such as __u64.

Had we done that, the pkeys change would not have been a problem either.

Is it too late to change that, is there any si_code=SEGV_BNDERR usage in 
user-space?

Thanks,

	Ingo

Re: [PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Dave Hansen]

On 03/01/2016 12:09 AM, Ingo Molnar wrote:
> Btw., what we should not have used in a modern user ABI are variable size 
> pointers:
> 
>                                 struct {
>                                         void __user *_lower;
>                                         void __user *_upper;
>                                 } _addr_bnd;
> 
> we should have used constant size structure elements for that, such as __u64.
> 
> Had we done that, the pkeys change would not have been a problem either.
> 
> Is it too late to change that, is there any si_code=SEGV_BNDERR usage in 
> user-space?

Yes, the libmpx code that gcc links in looks for si_code=SEGV_BNDERR
and, by default, will just just warn about the bounds exception (rather
than letting the app die.

This bit of code will be linked into basically anything using MPX.

Re: [PATCH] [v3] x86, pkeys: fix siginfo ABI breakage from new field

[Ingo Molnar]

> > A u64 was used for the protection key field in siginfo.  When the
> > containing union was aligned, this u64 unioned nicely with the
> > two 'void *'s in _addr_bnd.  But, on 32-bit, if the union was
> > unaligned, the u64 might grow the size of the union, breaking the
> > ABI for subsequent fields.

Btw., I think this explanation is incorrect, the layout of _addr_bnd is 
irrelevant.

What happened on some 32-bit platforms is the following: 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:

typedef struct siginfo {
        int si_signo;
        int si_errno;
        int si_code;

        union {
	...
        } _sifields;
} __ARCH_SI_ATTRIBUTES siginfo_t;

Note how the first 3 fields give us 12 bytes, so _sifields is not 8 naturally 
bytes aligned.

Before the _pkey field addition the largest element of _sifields (on 32-bit 
platforms) was 32 bits. With the u64 added, the minimum alignment requirement 
increased to 8 bytes on those (rare) 32-bit platforms. Thus GCC padded the space 
after si_code with 4 extra bytes, and shifted all _sifields offsets by 4 bytes - 
breaking the ABI of all of those remaining fields.

On 64-bit platforms this problem was hidden due to _sifields already having 
numerous fields with natural 8 bytes alignment (pointers).

If you agree with this analysis then mind updating the changelog accordingly?

Thanks,

	Ingo