| 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
| ||
|
Message-ID: <CAMuHMdVtbhGJ=nGbB21O73DcYM-4csyuJbx2NksN379ww6c1Gw@mail.gmail.com>
Date: Tue, 3 Apr 2018 09:30:36 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Eugene Syromiatnikov <esyr@...hat.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"Linux/m68k" <linux-m68k@...r.kernel.org>
Subject: Re: [GIT PULL] siginfo fix for v4.16-rc5
Hi Eric,
On Mon, Apr 2, 2018 at 10:17 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Eugene Syromiatnikov <esyr@...hat.com> writes:
>
>> So, the offset of the si_lower field is 20 at the current HEAD and was 18 at
>> commits v4.16-rc3~17^2 and v4.16-rc1~159^2~20. I believe this is due to
>> the fact that m68k uses 2-byte default alignment and not 4-byte.
>
> A 2-byte alignment for 4 byte pointers. That is a new one to me.
Not just for pointers, also for int and long.
And m68k is not the only architecture having such alignment rules.
> Euguene can you test the patch below. It should be fully robust against
> this kind of craziness. It certainly passes my BUILD_BUG_ON tests for
> m68k.
>
> Eric
>
> From: "Eric W. Biederman" <ebiederm@...ssion.com>
> Date: Mon, 2 Apr 2018 14:45:42 -0500
> Subject: [PATCH] signal: Correct the offset of si_pkey and si_lower in struct siginfo on m68k
>
> The change moving addr_lsb into the _sigfault union failed to take
> into account that _sigfault._addr_bnd._lower being a pointer forced
> the entire union to have pointer alignment. The fix for
> _sigfault._addr_bnd._lower having pointer alignment failed to take
> into account that m68k has a pointer alignment less than the size
> of a pointer. So simply making the padding members pointers changed
> the location of later members in the structure.
>
> Fix this by directly computing the needed size of the padding members,
> and making the padding members char arrays of the needed size. AKA
> if __alignof__(void *) is 1 sizeof(short) otherwise __alignof__(void *).
> Which should be exactly the same rules the compiler whould have
> used when computing the padding.
__alignof__(void *) is 2 not 1 on m68k.
> I have tested this change by adding BUILD_BUG_ONs to m68k to verify
> the offset of every member of struct siginfo, and with those testing
> that the offsets of the fields in struct siginfo is the same before
> I changed the generic _sigfault member and after the correction
> to the _sigfault member.
>
> I have also verified that the x86 with it's own BUILD_BUG_ONs to verify
> the offsets of the siginfo members also compiles cleanly.
>
> Cc: stable@...r.kernel.org
> Reported-by: Eugene Syromiatnikov <esyr@...hat.com>
> Fixes: 859d880cf544 ("signal: Correct the offset of si_pkey in struct siginfo")
> Fixes: b68a68d3dcc1 ("signal: Move addr_lsb into the _sigfault union for clarity")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
> include/linux/compat.h | 6 ++++--
> include/uapi/asm-generic/siginfo.h | 7 +++++--
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index e16d07eb08cf..d770e62632d7 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -221,6 +221,8 @@ typedef struct compat_siginfo {
> #ifdef __ARCH_SI_TRAPNO
> int _trapno; /* TRAP # which caused the signal */
> #endif
> +#define __COMPAT_ADDR_BND_PKEY_PAD (__alignof__(compat_uptr_t) < sizeof(short) ? \
> + sizeof(short) : __alignof__(compat_uptr_t))
On m68k, __alignof__(compat_uptr_t) == 2, so it will use
__alignof__(compat_uptr_t) padding bytes.
Note that while the test is wrong, the end result is correct :-)
Hence you could just use __alignof__(compat_uptr_t) padding bytes
unconditionally?
> union {
> /*
> * used when si_code=BUS_MCEERR_AR or
> @@ -229,13 +231,13 @@ typedef struct compat_siginfo {
> short int _addr_lsb; /* Valid LSB of the reported address. */
> /* used when si_code=SEGV_BNDERR */
> struct {
> - compat_uptr_t _dummy_bnd;
> + char _dummy_bnd[__COMPAT_ADDR_BND_PKEY_PAD];
> compat_uptr_t _lower;
> compat_uptr_t _upper;
> } _addr_bnd;
> /* used when si_code=SEGV_PKUERR */
> struct {
> - compat_uptr_t _dummy_pkey;
> + char _dummy_pkey[__COMPAT_ADDR_BND_PKEY_PAD];
> u32 _pkey;
> } _addr_pkey;
> };
> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> index 4b3520bf67ba..6d789648473d 100644
> --- a/include/uapi/asm-generic/siginfo.h
> +++ b/include/uapi/asm-generic/siginfo.h
> @@ -94,6 +94,9 @@ typedef struct siginfo {
> unsigned int _flags; /* see ia64 si_flags */
> unsigned long _isr; /* isr */
> #endif
> +
> +#define __ADDR_BND_PKEY_PAD (__alignof__(void *) < sizeof(short) ? \
> + sizeof(short) : __alignof__(void *))
Likewise.
> union {
> /*
> * used when si_code=BUS_MCEERR_AR or
> @@ -102,13 +105,13 @@ typedef struct siginfo {
> short _addr_lsb; /* LSB of the reported address */
> /* used when si_code=SEGV_BNDERR */
> struct {
> - void *_dummy_bnd;
> + char _dummy_bnd[__ADDR_BND_PKEY_PAD];
> void __user *_lower;
> void __user *_upper;
> } _addr_bnd;
> /* used when si_code=SEGV_PKUERR */
> struct {
> - void *_dummy_pkey;
> + char _dummy_pkey[__ADDR_BND_PKEY_PAD];
> __u32 _pkey;
> } _addr_pkey;
> };
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists