Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752575AbcDFSDV (ORCPT ); Wed, 6 Apr 2016 14:03:21 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35583 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751751AbcDFSDT (ORCPT ); Wed, 6 Apr 2016 14:03:19 -0400 MIME-Version: 1.0 In-Reply-To: <1459947782-5071-1-git-send-email-ed@abdsec.com> References: <1459947782-5071-1-git-send-email-ed@abdsec.com> Date: Wed, 6 Apr 2016 11:03:17 -0700 X-Google-Sender-Auth: -p-7hWF57nhntQsRqWph6gclp0s Message-ID: Subject: Re: [PATCH] KERNEL: resource: Fix bug on leakage in /proc/iomem file From: Kees Cook To: Emrah Demir Cc: LKML , Dan Rosenberg , "kernel-hardening@lists.openwall.com" , Linus Torvalds , Dave Jones 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: 3550 Lines: 99 On Wed, Apr 6, 2016 at 6:03 AM, Emrah Demir wrote: > From: Emrah Demir Hi! Thanks for sending this patch; I'm always glad to see new faces helping. :) I have a few comments inline and a larger suggestion at the end. > Even though KASLR is aiming to mitigate remote attacks, with a simple LFI vulnerability through a web application, local leaks become as important as remote ones. Be sure to 80-char wrap your commit logs (and code), as it makes reading it easier. Running your patch through scripts/checkpatch.pl would remind you: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #5: Even though KASLR is aiming to mitigate remote attacks, with a simple LFI vulnerability through a web application, local leaks become as important as remote ones. > On the KASLR enabled systems in order to achieve expected protection, some files are needed to edited/modified to prevent leaks. > > /proc/iomem file leaks offset of text section. By adding 0x80000000, Attackers can get _text base address. KASLR will be bypassed. Luckily, this will become less of a problem once the x86 virtual memory randomization code lands, but I think there's enough in this file that it should be protected. > > $ cat /proc/iomem | grep 'Kernel code' > 38600000-38b7fe92 : Kernel code > $ python -c 'print hex(0x38600000 + 0x80000000)' > 0xb8600000 > # cat /proc/kallsyms | grep 'T _text' > ffffffffb8600000 T _text > > By this patch after insertion resources, start and end address are zeroed. /proc/iomem and /proc/ioports sources, which use request_resource and insert_resource now shown as 0 value. > > Signed-off-by: Emrah Demir > --- > kernel/resource.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 2e78ead..5b9937e 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -321,6 +321,8 @@ int request_resource(struct resource *root, struct resource *new) > struct resource *conflict; > > conflict = request_resource_conflict(root, new); > + new->start = 0; > + new->end = 0; > return conflict ? -EBUSY : 0; > } > > @@ -864,6 +866,8 @@ int insert_resource(struct resource *parent, struct resource *new) > struct resource *conflict; > > conflict = insert_resource_conflict(parent, new); > + new->start = 0; > + new->end = 0; > return conflict ? -EBUSY : 0; > } > EXPORT_SYMBOL_GPL(insert_resource); This entirely eliminates the reporting, which I don't think is a good idea as developers and debuggers would like to be able to see this information. I would prefer that either /proc/iomem be unreadable to non-root users or that the values be reported using %pK to let the kptr_restrict control the output. diff --git a/kernel/resource.c b/kernel/resource.c index 2e78ead30934..8d5dd1dc9489 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -162,8 +162,8 @@ static const struct file_operations proc_iomem_operations = { static int __init ioresources_init(void) { - proc_create("ioports", 0, NULL, &proc_ioports_operations); - proc_create("iomem", 0, NULL, &proc_iomem_operations); + proc_create("ioports", S_IRUSR, NULL, &proc_ioports_operations); + proc_create("iomem", S_IRUSR, NULL, &proc_iomem_operations); return 0; } __initcall(ioresources_init); And if this breaks things, make the S_IRUSR conditional on the CONFIG_RANDOMIZE_BASE? -Kees -- Kees Cook Chrome OS & Brillo Security