lkml.org 
[lkml]   [2011]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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


\
 
 \ /
  Last update: 2011-07-24 19:43    [from the cache]
©2003-2011 Jasper Spaans