From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kroah-Hartman Subject: Re: [GIT PULL] hash addresses printed with %p Date: Thu, 30 Nov 2017 17:10:36 +0000 Message-ID: <20171130171036.GB31817@kroah.com> References: <20171129045927.GA6217@eros> <20171129210848.GF6217@eros> <20171130163235.GA27849@kroah.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20171130163235.GA27849-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Linus Torvalds Cc: "Tobin C. Harding" , Matt Fleming , Ard Biesheuvel , LKML , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-efi@vger.kernel.org On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote: > On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote: > > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds > > wrote: > > > > > > Not because %pK itself changed, but because the semantics of %p did. > > > The baseline moved, and the "safe" version did not. > > > > Btw, that baseline for me is now that I can do > > > > ./scripts/leaking_addresses.pl | wc -l > > 18 > > > > and of those 18 hits, six are false positives (looks like bitmaps in > > the uevent keys). > > > > The remaining 12 are from the EFI runtime map files > > (/sys/firmware/efi/runtime-map/*). They should presumably not be > > world-readable, but sadly the kset_create_and_add() helper seems to do > > that by default. > > > > I think the sysfs code makes it insanely too easy to make things > > world-readable. You try to be careful, and mark things read-only etc, > > but __ATTR_RO() jkust means S_IRUGO, which means world-readable. > > > > There seems to be no convenient model for kobjects having better > > permissions. Greg? > > They can just use __ATTR() which lets you set the exact mode settings > that are wanted. > > Something like the patch below, which breaks the build as the > map_attributes are "odd", but you get the idea. The EFI developers can > fix this up properly :) > > Note, this only accounts for 5 attributes, what is the whole list? Ah, it's the virt_addr file 12 times, I just ran it on my laptop: /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000 /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000 /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000 /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000 /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000 /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000 /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000 /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000 /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000 /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000 /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000 /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000 So changing it to use __ATTR() should fix this remaning leakage up. That is if we even really need to export these values at all. What does userspace do with them? Shouldn't they just be in debugfs instead? thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753375AbdK3RKd (ORCPT ); Thu, 30 Nov 2017 12:10:33 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:41602 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751383AbdK3RKb (ORCPT ); Thu, 30 Nov 2017 12:10:31 -0500 Date: Thu, 30 Nov 2017 17:10:36 +0000 From: Greg Kroah-Hartman To: Linus Torvalds Cc: "Tobin C. Harding" , Matt Fleming , Ard Biesheuvel , LKML , "linux-efi@vger.kernel.org" Subject: Re: [GIT PULL] hash addresses printed with %p Message-ID: <20171130171036.GB31817@kroah.com> References: <20171129045927.GA6217@eros> <20171129210848.GF6217@eros> <20171130163235.GA27849@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171130163235.GA27849@kroah.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote: > On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote: > > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds > > wrote: > > > > > > Not because %pK itself changed, but because the semantics of %p did. > > > The baseline moved, and the "safe" version did not. > > > > Btw, that baseline for me is now that I can do > > > > ./scripts/leaking_addresses.pl | wc -l > > 18 > > > > and of those 18 hits, six are false positives (looks like bitmaps in > > the uevent keys). > > > > The remaining 12 are from the EFI runtime map files > > (/sys/firmware/efi/runtime-map/*). They should presumably not be > > world-readable, but sadly the kset_create_and_add() helper seems to do > > that by default. > > > > I think the sysfs code makes it insanely too easy to make things > > world-readable. You try to be careful, and mark things read-only etc, > > but __ATTR_RO() jkust means S_IRUGO, which means world-readable. > > > > There seems to be no convenient model for kobjects having better > > permissions. Greg? > > They can just use __ATTR() which lets you set the exact mode settings > that are wanted. > > Something like the patch below, which breaks the build as the > map_attributes are "odd", but you get the idea. The EFI developers can > fix this up properly :) > > Note, this only accounts for 5 attributes, what is the whole list? Ah, it's the virt_addr file 12 times, I just ran it on my laptop: /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000 /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000 /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000 /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000 /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000 /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000 /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000 /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000 /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000 /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000 /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000 /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000 So changing it to use __ATTR() should fix this remaning leakage up. That is if we even really need to export these values at all. What does userspace do with them? Shouldn't they just be in debugfs instead? thanks, greg k-h