| Message ID | 1490811363-93944-3-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2017-03-29 at 11:15 -0700, Kees Cook wrote: > diff --git a/drivers/misc/lkdtm_perms.c b/drivers/misc/lkdtm_perms.c > index c7635a79341f..8fbadfa4cc34 100644 > --- a/drivers/misc/lkdtm_perms.c > +++ b/drivers/misc/lkdtm_perms.c > [...] > +/* This is marked __wr_rare, so it should ultimately be .rodata. */ > +static unsigned long wr_rare __wr_rare = 0xAA66AA66; > [...] > +void lkdtm_WRITE_RARE_WRITE(void) > +{ > + /* Explicitly cast away "const" for the test. */ wr_rare isn't actually declared const above though? I don't think __wr_rare includes a const, apologies if I missed it. OOI, if wr_rare _were_ const then can the compiler optimise the a pair of reads spanning the rare_write? i.e. adding const to the declaration above to get: static const unsigned long wr_rare __wr_rare = 0xAA66AA66; x = wr_read; rare_write(x, 0xf000baaa); y = wr_read; Is it possible that x == y == 0xaa66aa66 because gcc realises the x and y came from the same const location? Have I missed a clobber somewhere (I can't actually find a definition of __arch_rare_write_memcpy in this series so maybe it's there), or is such code expected to always cast away the const first? I suppose such constructs are rare in practice in the sorts of places where rare_write is appropriate, but with aggressive inlining it could occur as an unexpected trap for the unwary perhaps. Ian.
On Thu, Mar 30, 2017 at 2:34 AM, Ian Campbell <ijc@hellion.org.uk> wrote: > On Wed, 2017-03-29 at 11:15 -0700, Kees Cook wrote: >> diff --git a/drivers/misc/lkdtm_perms.c b/drivers/misc/lkdtm_perms.c >> index c7635a79341f..8fbadfa4cc34 100644 >> --- a/drivers/misc/lkdtm_perms.c >> +++ b/drivers/misc/lkdtm_perms.c >> [...] >> +/* This is marked __wr_rare, so it should ultimately be .rodata. */ >> +static unsigned long wr_rare __wr_rare = 0xAA66AA66; >> [...] >> +void lkdtm_WRITE_RARE_WRITE(void) >> +{ >> + /* Explicitly cast away "const" for the test. */ > > wr_rare isn't actually declared const above though? I don't think > __wr_rare includes a const, apologies if I missed it. Yeah, good point. I think this was a left-over from an earlier version where I'd forgotten about that detail. > OOI, if wr_rare _were_ const then can the compiler optimise the a pair > of reads spanning the rare_write? i.e. adding const to the declaration > above to get: > > static const unsigned long wr_rare __wr_rare = 0xAA66AA66; > x = wr_read; > rare_write(x, 0xf000baaa); > y = wr_read; > > Is it possible that x == y == 0xaa66aa66 because gcc realises the x and > y came from the same const location? Have I missed a clobber somewhere > (I can't actually find a definition of __arch_rare_write_memcpy in this > series so maybe it's there), or is such code expected to always cast > away the const first? > > I suppose such constructs are rare in practice in the sorts of places > where rare_write is appropriate, but with aggressive inlining it could > occur as an unexpected trap for the unwary perhaps. Right, __wr_rare is actually marked as .data..ro_after_init, which gcc effectively ignores (thinking it's part of .data), but the linker script later movies this section into the read-only portion with .rodata. As a result, the compiler treats it as writable, but the storage location is actually read-only. (And, AIUI, the constify plugin makes things read-only in a similar way, though I think it's more subtle but still avoids the const-optimization dangers.) -Kees
diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h index 67d27be60405..d1fd5aefa235 100644 --- a/drivers/misc/lkdtm.h +++ b/drivers/misc/lkdtm.h @@ -39,6 +39,7 @@ void lkdtm_READ_BUDDY_AFTER_FREE(void); void __init lkdtm_perms_init(void); void lkdtm_WRITE_RO(void); void lkdtm_WRITE_RO_AFTER_INIT(void); +void lkdtm_WRITE_RARE_WRITE(void); void lkdtm_WRITE_KERN(void); void lkdtm_EXEC_DATA(void); void lkdtm_EXEC_STACK(void); diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c index b9a4cd4a9b68..ac8a55947189 100644 --- a/drivers/misc/lkdtm_core.c +++ b/drivers/misc/lkdtm_core.c @@ -219,6 +219,7 @@ struct crashtype crashtypes[] = { CRASHTYPE(ACCESS_USERSPACE), CRASHTYPE(WRITE_RO), CRASHTYPE(WRITE_RO_AFTER_INIT), + CRASHTYPE(WRITE_RARE_WRITE), CRASHTYPE(WRITE_KERN), CRASHTYPE(REFCOUNT_SATURATE_INC), CRASHTYPE(REFCOUNT_SATURATE_ADD), diff --git a/drivers/misc/lkdtm_perms.c b/drivers/misc/lkdtm_perms.c index c7635a79341f..8fbadfa4cc34 100644 --- a/drivers/misc/lkdtm_perms.c +++ b/drivers/misc/lkdtm_perms.c @@ -20,12 +20,15 @@ /* This is non-const, so it will end up in the .data section. */ static u8 data_area[EXEC_SIZE]; -/* This is cost, so it will end up in the .rodata section. */ +/* This is const, so it will end up in the .rodata section. */ static const unsigned long rodata = 0xAA55AA55; /* This is marked __ro_after_init, so it should ultimately be .rodata. */ static unsigned long ro_after_init __ro_after_init = 0x55AA5500; +/* This is marked __wr_rare, so it should ultimately be .rodata. */ +static unsigned long wr_rare __wr_rare = 0xAA66AA66; + /* * This just returns to the caller. It is designed to be copied into * non-executable memory regions. @@ -103,6 +106,20 @@ void lkdtm_WRITE_RO_AFTER_INIT(void) *ptr ^= 0xabcd1234; } +void lkdtm_WRITE_RARE_WRITE(void) +{ + /* Explicitly cast away "const" for the test. */ + unsigned long *ptr = (unsigned long *)&wr_rare; + + pr_info("attempting good rare write at %p\n", ptr); + rare_write(*ptr, 0x11335577); + if (wr_rare != 0x11335577) + pr_warn("Yikes: wr_rare did not actually change!\n"); + + pr_info("attempting bad rare write at %p\n", ptr); + *ptr ^= 0xbcd12345; +} + void lkdtm_WRITE_KERN(void) { size_t size;
This adds the WRITE_RARE_WRITE test to validate variables marked with __wr_rare. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/misc/lkdtm.h | 1 + drivers/misc/lkdtm_core.c | 1 + drivers/misc/lkdtm_perms.c | 19 ++++++++++++++++++- 3 files changed, 20 insertions(+), 1 deletion(-)