Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 24 Jul 2011 10:39:25 -0700 | Subject | Re: [PATCH] x86, AMD: Correct F15h IC aliasing issue |
| |
On Sun, Jul 24, 2011 at 10:22 AM, Borislav Petkov <bp@amd64.org> wrote: > >> So at a MINIMUM, I would say that this is acceptable only when the >> process doing the allocation hasn't got ASLR disabled. > > I guess I could look at randomize_va_space before enabling it.
That's not what I meant - I meant the per-process PF_RANDOMIZE and ADDR_NO_RANDOMIZE personality flags (although the global "randomize_va_space" thing obviously is one input to that too)
In fact, if 99% of your problem is ASLR-induced, might I suggest just making the whole thing a tweak to ASLR instead, and not use ASLR for bits 14:12? That should be fundamentally much safer: it doesn't change any semantics at all, it just makes for slightly less random bits to be used.
So I really think that you might be *much* better off just changing mmap_rnd(), and nothing else. Just make *that* mask off the three low bits of the random address, ie something like
diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c index 1dab5194fd9d..6b62ab5a5ae1 100644 --- a/arch/x86/mm/mmap.c +++ b/arch/x86/mm/mmap.c @@ -90,6 +90,9 @@ static unsigned long mmap_rnd(void) rnd = (long)get_random_int() % (1<<8); else rnd = (long)(get_random_int() % (1<<28)); + + if (avoid_aliasing_in_bits_14_12) + rnd &= ~7; } return rnd << PAGE_SHIFT; }
would be fundamentally very safe - it would already take all our current anti-randomization code into account.
No?
> But this won't address the case where one of the processes was created > with ASLR off and the other with ASLR on and they map the same library > at VAs differing at bits [14:12].
I wouldn't worry about some corner-case like that _nearly_ as much as worrying about the non-ASLR process working at all.
> Yeah, I like the BITS() thing - will change. I actually have a similar > macro GENMASK(o, hi) in <drivers/edac/amd64_edac.h> - I should move it > to <linux/bitops.h> and rename it to BITS().
So it may be that BITS() is much too generic a name, and will cause problems. A quick "git grep -w BITS" certainly finds a fair number of hits. So I don't think it's usable as-is, it was meant more as pseudo-code.
>> Changing address space layout is not a small decision. > > I suspected as much - thus the boot option to disable it.
I understand that the boot option is worth it, but since we _already_ have a way to mark binaries as not wanting address space layout changes, I really think it should use that as the primary method. When that bit is set, I think it's a big hint that the process is "fragile" wrt address space changes.
A boot option might be left as a last ditch thing, but I don't think it should be the primary model.
Linus
| |