From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752264AbcAFCtf (ORCPT ); Tue, 5 Jan 2016 21:49:35 -0500 Received: from mail-pf0-f176.google.com ([209.85.192.176]:36816 "EHLO mail-pf0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751976AbcAFCte (ORCPT ); Tue, 5 Jan 2016 21:49:34 -0500 Subject: Re: [RFC][PATCH 7/7] lkdtm: Add READ_AFTER_FREE test To: Kees Cook References: <1450755641-7856-1-git-send-email-laura@labbott.name> <1450755641-7856-8-git-send-email-laura@labbott.name> Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Linux-MM , LKML , "kernel-hardening@lists.openwall.com" , Arnd Bergmann , Greg Kroah-Hartman From: Laura Abbott Message-ID: <568C80BC.4080507@labbott.name> Date: Tue, 5 Jan 2016 18:49:32 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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