Re: [GIT PULL] hash addresses printed with %p

[Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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
> > <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> 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