[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160831164119.GA6633@leverpostej>
Date: Wed, 31 Aug 2016 17:41:57 +0100
From: Mark Rutland <mark.rutland@....com>
To: Kees Cook <keescook@...omium.org>
Cc: Julia Lawall <julia.lawall@...6.fr>, Joe Perches <joe@...ches.com>,
Fengguang Wu <fengguang.wu@...el.com>,
LKML <linux-kernel@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
Glenn Wurster <gwurster@...ckberry.com>,
PaX Team <pageexec@...email.hu>
Subject: Re: constification and cocci / kernel build test robot ?
Hi Kees,
On Wed, Aug 31, 2016 at 10:44:28AM -0400, Kees Cook wrote:
> On Wed, Aug 31, 2016 at 6:08 AM, Mark Rutland <mark.rutland@....com> wrote:
> > On Tue, Aug 30, 2016 at 02:50:36PM -0400, Kees Cook wrote:
> >> The structures that should get the greatest level of attention are
> >> those that contain function pointers. The "constify" gcc plugin from
> >> PaX/Grsecurity does this, but it uses a big hammer: it moves all of
> >> them const even if they receive assignment. To handle this, there is
> >> the concept of an open/close method to gain temporary access to the
> >> structure. For example:
> >>
> >> drivers/cdrom/cdrom.c:
> >>
> >> int register_cdrom(...) {
> >> ...
> >> if (!cdo->generic_packet) {
> >> pax_open_kernel();
> >> const_cast(cdo->generic_packet) = cdrom_dummy_generic_packet;
> >> pax_close_kernel();
> >> }
> >>
> >> (The "const_cast" here is just a macro to convince gcc it's only to
> >> write to a const value, so really it should maybe be called
> >> "unconst_cast", but whatever...)
> >
> > Just to check, are they actually marked as const, or some pseudo-const
> > like __ro_after_init?
>
> The plugin marks them actually const, so the const_cast() is needed to
> "forget" that marking and treat it as a normal variable. (And PaX Team
> noted that this is the name used in C++ already.)
>From having a look around, my understanding is that with C++ this is
only valid if the underlying object is not const (i.e. it's only valid
to remove constness from a pointer or reference which had itself added
constness to a non-const object).
I see that GCC is happy to constant-fold function pointers in const
objects it has visibility of; example below. Making the objects
themselves const is bound to lead to fragility (e.g. static inline
functions in a header behaving differently from related functions in
another file).
Have I misunderstood something?
Note: For the below I manually fixed up the symbol resolution in ret_a,
as by default objdump misleadingly reports <intfunc> rather than <a>.
---->8----
[mark@...erpostej:~]% cat const-test.c
int intfunc(int arg)
{
return arg;
}
struct foo {
int (*func)(int arg);
};
struct foo a = {
.func = intfunc,
};
const struct foo b = {
.func = intfunc,
};
extern const struct foo c;
int ret_a(int val)
{
return a.func(val);
}
int ret_b(int val)
{
return b.func(val);
}
int ret_c(int val)
{
return c.func(val);
}
[mark@...erpostej:~]% uselinaro 15.08 aarch64-linux-gnu-gcc -O3 -c const-test.c
[mark@...erpostej:~]% uselinaro 15.08 aarch64-linux-gnu-objdump -d const-test.o
const-test.o: file format elf64-littleaarch64
Disassembly of section .text:
0000000000000000 <intfunc>:
0: d65f03c0 ret
4: d503201f nop
0000000000000008 <ret_a>:
8: 90000001 adrp x1, 0 <a>
c: f9400021 ldr x1, [x1]
10: d61f0020 br x1
14: d503201f nop
0000000000000018 <ret_b>:
18: d65f03c0 ret
1c: d503201f nop
0000000000000020 <ret_c>:
20: 90000001 adrp x1, 0 <c>
24: f9400021 ldr x1, [x1]
28: d61f0020 br x1
2c: d503201f nop
---->8----
> > I can imagine a number of issues with casting a real const away (e.g. if
> > the compiler decides to cache the value in a register and decides since
> > it is const it need not hazard with a memory clobber).
>
> AFAIK, this hasn't caused problems. PaX Team may be able to speak more
> to this...
Per the above, and assuming that I haven't missed something, I cannot
see how this can be safe, even if it happens to have worked so far.
Hopefully I'm just being thick, and someone can correct me. :)
[...]
> >> This makes sure there aren't races with other CPUs to write things,
> >> and that it's harder to use for an attack since with the "make
> >> writable" code is always followed by a "make read-only" action (i.e.
> >> not separate functions that could be used as a trivial ROP gadget).
> >
> > FWIW, on that front we may want to look into reworking the fixmap code.
> > For at least arm, arm64, microblaze, and powerpc, __set_fixmap is a C
> > function that might offer a trivial ROP gadget for creating a RW
> > mapping.
> >
> > Other architectures have that as a static inline in a header, which
> > ensures that it's folded into callers, making it less trivial to reuse.
>
> Even with the inlining, it's just making things slightly harder, since
> there is still a memory write happening, so a careful setup before
> calling it can still be used as a ROP gadget.
Sure. Hence "less trivial". ;)
If it's not likely to be a noticeable improvement, I'm happy to leave
that as-is. I just thought it was worth mentioning.
Thanks,
Mark.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
