linux-kernel - Re: [PATCH v2 2/4] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access

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]

Date:   Fri, 29 Jun 2018 23:44:44 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Dave Hansen <dave.hansen@...el.com>
cc:     Fenghua Yu <fenghua.yu@...el.com>, Ingo Molnar <mingo@...hat.com>,
        H Peter Anvin <hpa@...or.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Alan Cox <alan@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rafael Wysocki <rafael.j.wysocki@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 2/4] x86/split_lock: Align x86_capability to unsigned
 long to avoid split locked access

On Fri, 29 Jun 2018, Dave Hansen wrote:

> On 06/29/2018 01:38 PM, Fenghua Yu wrote:
> > How to handle data that is used in generic code which can be used on
> > non-Intel platform? For exmple, if I do this change for struct efi in
> > include/linux/efi.h because set_bit() sets bits in efi.flags:
> > -       unsigned long flags;
> > +       unsigned long flags __aligned(unsigned long);
> >  } efi;
> > 
> > People may argue that the alignment unnecessarily increases size of 'efi'
> > on non-Intel platform which doesn't have split lock issue. Do we care this
> > argument?
> 
> Unaligned memory accesses are bad, pretty much universally.  This is a
> general good practice that we should have been doing anyway.  Let folks
> complain.  Don't let it stop you.
> 
> Also, look at the size of that structure.  Look at how many pointers it
> has.  Do you think *anyone* is going to complain about an extra 4 bytes
> in a 400-byte structure?

But in the above case the compiler does already the right thing. Why?
Because struct members are aligned to their natural alignment unless the
struct is explicitely marked 'packed'. In that case the programmer has to
take care of the alignment.

Just look at it with pahole:

	struct efi_memory_map      memmap;               /*   280    56 */

	/* XXX last struct has 7 bytes of padding */

	/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
	long unsigned int          flags;                /*   336     8 */

The issue with the capability arrays is that the data type is u32 which has
the natural alignment of 4 byte, while unsigned long has 8 byte on 64bit.

So just slapping blindly aligned(unsigned long) to anything which is
accessed by locked instructions is pointless.

Thanks,

	tglx