From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com References: <1450755641-7856-1-git-send-email-laura@labbott.name> <1450755641-7856-8-git-send-email-laura@labbott.name> From: Laura Abbott Message-ID: <568C80BC.4080507@labbott.name> Date: Tue, 5 Jan 2016 18:49:32 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [kernel-hardening] Re: [RFC][PATCH 7/7] lkdtm: Add READ_AFTER_FREE test To: Kees Cook Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Linux-MM , LKML , "kernel-hardening@lists.openwall.com" , Arnd Bergmann , Greg Kroah-Hartman List-ID: On 1/5/16 4:15 PM, Kees Cook wrote: > On Mon, Dec 21, 2015 at 7:40 PM, Laura Abbott wrote: >> >> In a similar manner to WRITE_AFTER_FREE, add a READ_AFTER_FREE >> test to test free poisoning features. Sample output when >> no poison is present: >> >> [ 20.222501] lkdtm: Performing direct entry READ_AFTER_FREE >> [ 20.226163] lkdtm: Freed val: 12345678 >> >> with poison: >> >> [ 24.203748] lkdtm: Performing direct entry READ_AFTER_FREE >> [ 24.207261] general protection fault: 0000 [#1] SMP >> [ 24.208193] Modules linked in: >> [ 24.208193] CPU: 0 PID: 866 Comm: sh Not tainted 4.4.0-rc5-work+ #108 >> >> Cc: Arnd Bergmann >> Cc: Greg Kroah-Hartman >> Signed-off-by: Laura Abbott >> --- >> drivers/misc/lkdtm.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c >> index 11fdadc..c641fb7 100644 >> --- a/drivers/misc/lkdtm.c >> +++ b/drivers/misc/lkdtm.c >> @@ -92,6 +92,7 @@ enum ctype { >> CT_UNALIGNED_LOAD_STORE_WRITE, >> CT_OVERWRITE_ALLOCATION, >> CT_WRITE_AFTER_FREE, >> + CT_READ_AFTER_FREE, >> CT_SOFTLOCKUP, >> CT_HARDLOCKUP, >> CT_SPINLOCKUP, >> @@ -129,6 +130,7 @@ static char* cp_type[] = { >> "UNALIGNED_LOAD_STORE_WRITE", >> "OVERWRITE_ALLOCATION", >> "WRITE_AFTER_FREE", >> + "READ_AFTER_FREE", >> "SOFTLOCKUP", >> "HARDLOCKUP", >> "SPINLOCKUP", >> @@ -417,6 +419,33 @@ static void lkdtm_do_action(enum ctype which) >> memset(data, 0x78, len); >> break; >> } >> + case CT_READ_AFTER_FREE: { >> + int **base; >> + int *val, *tmp; >> + >> + base = kmalloc(1024, GFP_KERNEL); >> + if (!base) >> + return; >> + >> + val = kmalloc(1024, GFP_KERNEL); >> + if (!val) >> + return; > > For both of these test failure return, I think there should be a > pr_warn too (see CT_EXEC_USERSPACE). > I was going by the usual rule that messages on memory failures are redundant because something somewhere else is going to be printing out error messages. >> + >> + *val = 0x12345678; >> + >> + /* >> + * Don't just use the first entry since that's where the >> + * freelist goes for the slab allocator >> + */ >> + base[1] = val; > > Maybe just aim at the middle, in case allocator freelist tracking ever > grows? base[1024/sizeof(int)/2] or something? > Good point. >> + kfree(base); >> + >> + tmp = base[1]; >> + pr_info("Freed val: %x\n", *tmp); > > Instead of depending on the deref to fail, maybe just use a simple > BUG_ON to test that the value did actually change? Or, change the > pr_info to "Failed to Oops when reading freed value: ..." just to be > slightly more verbose about what failed? > I'll come up with something to be more explicit here. Thanks, Laura