Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933449AbdDAAED (ORCPT ); Fri, 31 Mar 2017 20:04:03 -0400 Received: from mail-io0-f178.google.com ([209.85.223.178]:33067 "EHLO mail-io0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933112AbdDAAEB (ORCPT ); Fri, 31 Mar 2017 20:04:01 -0400 MIME-Version: 1.0 In-Reply-To: <20170331143317.3865149a6b6112f0d1a63499@linux-foundation.org> References: <20170331164028.GA118828@beast> <20170331143317.3865149a6b6112f0d1a63499@linux-foundation.org> From: Kees Cook Date: Fri, 31 Mar 2017 17:04:00 -0700 X-Google-Sender-Auth: r3y7GTumNLo12RRqpUboXNwVTck Message-ID: Subject: Re: [PATCH] mm: Add additional consistency check To: Andrew Morton Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Linux-MM , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1690 Lines: 44 On Fri, Mar 31, 2017 at 2:33 PM, Andrew Morton wrote: > On Fri, 31 Mar 2017 09:40:28 -0700 Kees Cook wrote: > >> As found in PaX, this adds a cheap check on heap consistency, just to >> notice if things have gotten corrupted in the page lookup. > > "As found in PaX" isn't a very illuminating justification for such a > change. Was there a real kernel bug which this would have exposed, or > what? I don't know off the top of my head, but given the kinds of heap attacks I've been seeing, I think this added consistency check is worth it given how inexpensive it is. When heap metadata gets corrupted, we can get into nasty side-effects that can be attacker-controlled, so better to catch obviously bad states as early as possible. >> --- a/mm/slab.h >> +++ b/mm/slab.h >> @@ -384,6 +384,7 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x) >> return s; >> >> page = virt_to_head_page(x); >> + BUG_ON(!PageSlab(page)); >> cachep = page->slab_cache; >> if (slab_equal_or_root(cachep, s)) >> return cachep; > > BUG_ON might be too severe. I expect the kindest VM_WARN_ON_ONCE() > would suffice here, but without more details it is hard to say. So, WARN isn't enough to protect the kernel (execution continues and the memory is still dereferenced for malicious purposes, etc). Perhaps use CHECK_DATA_CORRUPTION() here, which can either WARN and take a "safe" path, or BUG (depending on config paranoia of the builder). I've got a series adding it in a number of other places, so I could add this patch to that series? -Kees -- Kees Cook Pixel Security