[GIT PULL] hash addresses printed with %p

[GIT PULL] hash addresses printed with %p

[Tobin C. Harding]

The following changes since commit 4fbd8d194f06c8a3fd2af1ce560ddb31f7ec8323:

  Linux 4.15-rc1 (2017-11-26 16:01:47 -0800)

are available in the git repository at:

  git://github.com/tcharding/linux.git tags/printk-hash-pointer-4.15-rc2

for you to fetch changes up to 6424f6bb432752c7eb90cbeeb1c31d6125bba39a:

  kasan: use %px to print addresses instead of %p (2017-11-29 12:13:16 +1100)

----------------------------------------------------------------
printk hashing patches for 4.15-rc2

Here is the patch set that implements hashing of printk specifier
%p. First we have two clean up patches then we do the hashing. Hashing
is done via the SipHash algorithm. The next patch adds printk specifier
%px for printing pointers when we _really_ want to see the address i.e
%px is functionally equivalent to %lx. Final patch in the set fixes
KASAN since we break it by hashing %p.

For the record here is the justification for the series.

Currently there exist approximately 14 000 places in the Kernel where
addresses are being printed using an unadorned %p. This potentially
leaks sensitive information about the Kernel layout in memory. Many of
these calls are stale, instead of fixing every call we can hash the
address by default before printing. We then add %px to provide a way to
print the actual address. Although this is achievable using %lx, using
%px will assist us if we ever want to change pointer printing
behaviour. %px is more uniquely grep'able (there are already > 50 000
uses of %lx).

The added advantage of hashing %p is that security is now opt-out, if
you _really_ want the address you have to work a little harder and use
%px.

This will of course break some users, forcing code printing needed
addresses to be updated.

Signed-off-by: Tobin C. Harding <me@tobin.cc>

----------------------------------------------------------------
Tobin C. Harding (5):
      docs: correct documentation for %pK
      vsprintf: refactor %pK code out of pointer()
      printk: hash addresses printed with %p
      vsprintf: add printk specifier %px
      kasan: use %px to print addresses instead of %p

 Documentation/printk-formats.txt |  31 ++++++-
 lib/test_printf.c                | 108 ++++++++++++++--------
 lib/vsprintf.c                   | 194 +++++++++++++++++++++++++++++----------
 mm/kasan/report.c                |   8 +-
 scripts/checkpatch.pl            |   2 +-
 5 files changed, 248 insertions(+), 95 deletions(-)

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

[Linus Torvalds]

On Tue, Nov 28, 2017 at 8:59 PM, Tobin C. Harding <me@tobin.cc> wrote:
>
>   git://github.com/tcharding/linux.git tags/printk-hash-pointer-4.15-rc2

Bah.

What I didn't realize until after pulling this and testing, is that it
completely breaks '%pK'.

We've marked various sensitive pointers with %pK, but that is now
_less_ secure than %p is, since it doesn't do the hashing because of
how you refactored the %pK code out of 'pointer()' into its own
function.

So now %pK ends up using the plain "number()" function. Reading
through the series I hadn't noticed that the refactoring ended up
messing with that.

I'll fix it up somehow.

                Linus

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

[Linus Torvalds]

On Wed, Nov 29, 2017 at 11:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> What I didn't realize until after pulling this and testing, is that it
> completely breaks '%pK'.
>
> We've marked various sensitive pointers with %pK, but that is now
> _less_ secure than %p is, since it doesn't do the hashing because of
> how you refactored the %pK code out of 'pointer()' into its own
> function.
>
> So now %pK ends up using the plain "number()" function. Reading
> through the series I hadn't noticed that the refactoring ended up
> messing with that.
>
> I'll fix it up somehow.

I ended up just doing this:

            case 'K':
    +               if (!kptr_restrict)
    +                       break;
                    return restricted_pointer(buf, end, ptr, spec);

which basically says that "if kptr_restrict isn't set, %pK is the same as %p".

Now, I feel that we should probably get rid of 'restricted_pointer()'
entirely, since now the regular '%p' is arguably safer than '%pK' is,
but I also didn't want to mess with the case that I have never used
and that most distros don't seem to set.

Alternatively, we might make the 'K' behavior of clearing the pointer
be in addition to the other flags, so that you could do '%pxK' and get
the old %pK behavior. But since I am not a huge fan of %pK to begin
with, I can't find it in myself to care too much.

So I'll leave that for Kees & co to decide on. Comments?

                  Linus

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

[Joe Perches]

On Wed, 2017-11-29 at 11:39 -0800, Linus Torvalds wrote:
> On Wed, Nov 29, 2017 at 11:22 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > 
> > What I didn't realize until after pulling this and testing, is that it
> > completely breaks '%pK'.
> > 
> > We've marked various sensitive pointers with %pK, but that is now
> > _less_ secure than %p is, since it doesn't do the hashing because of
> > how you refactored the %pK code out of 'pointer()' into its own
> > function.
> > 
> > So now %pK ends up using the plain "number()" function. Reading
> > through the series I hadn't noticed that the refactoring ended up
> > messing with that.
> > 
> > I'll fix it up somehow.
> 
> I ended up just doing this:
> 
>             case 'K':
>     +               if (!kptr_restrict)
>     +                       break;
>                     return restricted_pointer(buf, end, ptr, spec);
> 
> which basically says that "if kptr_restrict isn't set, %pK is the same as %p".
> 
> Now, I feel that we should probably get rid of 'restricted_pointer()'
> entirely, since now the regular '%p' is arguably safer than '%pK' is,
> but I also didn't want to mess with the case that I have never used
> and that most distros don't seem to set.
> 
> Alternatively, we might make the 'K' behavior of clearing the pointer
> be in addition to the other flags, so that you could do '%pxK' and get
> the old %pK behavior. But since I am not a huge fan of %pK to begin
> with, I can't find it in myself to care too much.
> 
> So I'll leave that for Kees & co to decide on. Comments?

I'd prefer a global sed of '%pK' to '%pxK' and remove '%pK' altogether

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

[Linus Torvalds]

On Wed, Nov 29, 2017 at 12:54 PM, Joe Perches <joe@perches.com> wrote:
>
> I'd prefer a global sed of '%pK' to '%pxK' and remove '%pK' altogether

No, we really don't want %pK to become %pxK.

Most of the %pK users absolutely do not want the real hex address.
They are things like the socket pointers in /proc etc. The exact thing
that the hashing does well, and that %pK was so useless for.

Anyway, with the current tree, the "leaking_addresses script gives
almost no results for me (and I have kptr_restrict set to 0).

I haven't seen any breakage, but I may change my mind if bug reports
end up being illegible. And maybe I just haven't triggered anything
that might use a pointer.

Annotated perf disassembly is broken for me right now, but that
happens to me with alarming regularity because perf uses the wrong
vmlinux file or something, so it is probably not related (the kernel
symbol show up properly in the profile, I just don't get the
disassembly).

               Linus

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

[Kees Cook]

On Wed, Nov 29, 2017 at 11:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Nov 29, 2017 at 11:22 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> What I didn't realize until after pulling this and testing, is that it
>> completely breaks '%pK'.
>>
>> We've marked various sensitive pointers with %pK, but that is now
>> _less_ secure than %p is, since it doesn't do the hashing because of
>> how you refactored the %pK code out of 'pointer()' into its own
>> function.
>>
>> So now %pK ends up using the plain "number()" function. Reading
>> through the series I hadn't noticed that the refactoring ended up
>> messing with that.
>>
>> I'll fix it up somehow.
>
> I ended up just doing this:
>
>             case 'K':
>     +               if (!kptr_restrict)
>     +                       break;
>                     return restricted_pointer(buf, end, ptr, spec);
>
> which basically says that "if kptr_restrict isn't set, %pK is the same as %p".
>
> Now, I feel that we should probably get rid of 'restricted_pointer()'
> entirely, since now the regular '%p' is arguably safer than '%pK' is,
> but I also didn't want to mess with the case that I have never used
> and that most distros don't seem to set.

kptr_restrict=0 is now much safer, yes (i.e. %pK becomes hashed %p).
Strictly speaking, kptr_restrict > 0 is "better" than hashed %p, in
that it only says "0".

> Alternatively, we might make the 'K' behavior of clearing the pointer
> be in addition to the other flags, so that you could do '%pxK' and get
> the old %pK behavior. But since I am not a huge fan of %pK to begin
> with, I can't find it in myself to care too much.
>
> So I'll leave that for Kees & co to decide on. Comments?

I'm not hugely attached to %pK, but retaining its ability to zero out
the result would be nice.

(If we in the future we have a toggle for %p that switches it from
hashing to zeroing, then we could entirely drop %pK.)

-Kees

-- 
Kees Cook
Pixel Security

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

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 11:22:29AM -0800, Linus Torvalds wrote:
> On Tue, Nov 28, 2017 at 8:59 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >
> >   git://github.com/tcharding/linux.git tags/printk-hash-pointer-4.15-rc2
> 
> Bah.

Sorry for creating extra work for you.

> What I didn't realize until after pulling this and testing, is that it
> completely breaks '%pK'.

If you haven't wasted enough time on this can you tell me what you mean
by 'completely breaks %pK'?

If I am at fault I do not want to repeat the same mistake again.

I have just re-run my tests and it passes so something must be wrong
with my tests or method. I wrote a module to print various pointers
using %pK (same module that tests the hashing stuff), built the kernel
with the patch set applied then booted the kernel in a VM and inserted
the module (kptr_restrict==0). Confirmed that addresses were
displayed. Then I set kptr_restrict to 2 and re-inserted the
module. Confirmed that pointers were zeroed out when printed with %pK.

> We've marked various sensitive pointers with %pK, but that is now
> _less_ secure than %p is, since it doesn't do the hashing because of
> how you refactored the %pK code out of 'pointer()' into its own
> function.

Oh, I think I get it. You mean that it is better to hash the address for
%pK (kpt_restrict==0) than to zero it out?

> So now %pK ends up using the plain "number()" function. Reading
> through the series I hadn't noticed that the refactoring ended up
> messing with that.
> 
> I'll fix it up somehow.

(I saw the fix in the next email)

thanks,
Tobin.

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

[Linus Torvalds]

On Wed, Nov 29, 2017 at 1:08 PM, Tobin C. Harding <me@tobin.cc> wrote:
>
> If you haven't wasted enough time on this can you tell me what you mean
> by 'completely breaks %pK'?

The whole point of %pK is that it's a "safer" %p that doesn't leak
information if you set kptr_restrict.

With that patch-set, it now leaks _more_ information than %p when
kptr_restrict isn't set, so %pK went from "be more careful than %p" to
"be wildly less careful than %p".

Not because %pK itself changed, but because the semantics of %p did.
The baseline moved, and the "safe" version did not.

                 Linus

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

[Tobin C. Harding]

On Wed, Nov 29, 2017 at 01:14:38PM -0800, Linus Torvalds wrote:
> On Wed, Nov 29, 2017 at 1:08 PM, Tobin C. Harding <me@tobin.cc> wrote:
> >
> > If you haven't wasted enough time on this can you tell me what you mean
> > by 'completely breaks %pK'?
> 
> The whole point of %pK is that it's a "safer" %p that doesn't leak
> information if you set kptr_restrict.
> 
> With that patch-set, it now leaks _more_ information than %p when
> kptr_restrict isn't set, so %pK went from "be more careful than %p" to
> "be wildly less careful than %p".
> 
> Not because %pK itself changed, but because the semantics of %p did.
> The baseline moved, and the "safe" version did not.

Oh sweet, I was shitting bricks when I read your email :)

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

[Linus Torvalds]

On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
<torvalds@linux-foundation.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?

              Linus

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

[Greg Kroah-Hartman]

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@linux-foundation.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?

thanks,

greg k-h

diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
index 8e64b77aeac9..09444964c8d7 100644
--- a/drivers/firmware/efi/runtime-map.c
+++ b/drivers/firmware/efi/runtime-map.c
@@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
 	return map_attr->show(entry, buf);
 }
 
-static struct map_attribute map_type_attr = __ATTR_RO(type);
-static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
-static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
-static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
-static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
+static struct map_attribute map_type_attr = __ATTR(type, 0400, type_show, NULL);
+static struct map_attribute map_phys_addr_attr  = __ATTR(phys_addr, 0400, phys_addr_show, NULL);
+static struct map_attribute map_virt_addr_attr  = __ATTR(virt_addr, 0400, virt_addr_show, NULL);
+static struct map_attribute map_num_pages_attr  = __ATTR(num_pages, 0400, num_pages_show, NULL);
+static struct map_attribute map_attribute_attr  = __ATTR(attribute, 0400, attribute_show, NULL);
 
 /*
  * These are default attributes that are added for every memmap entry.

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

[Greg Kroah-Hartman]

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@linux-foundation.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

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

[Ard Biesheuvel]

On 30 November 2017 at 17:10, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> 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@linux-foundation.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?
>

These are the virtual mappings UEFI firmware regions, which must
remain in the same place across kexec reboots. So kexec tooling
consumes this information and passes it on to the incoming kernel in
some way.

Note that these are not kernel addresses, so while I agree they should
not be world readable, they won't give you any clue as to where the
kernel itself is mapped.

So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
If so, I'll code up a patch.

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

[Greg Kroah-Hartman]

On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > 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@linux-foundation.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?
> >
> 
> These are the virtual mappings UEFI firmware regions, which must
> remain in the same place across kexec reboots. So kexec tooling
> consumes this information and passes it on to the incoming kernel in
> some way.
> 
> Note that these are not kernel addresses, so while I agree they should
> not be world readable, they won't give you any clue as to where the
> kernel itself is mapped.
> 
> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> If so, I'll code up a patch.

If these pointers are not "real", I recommend just leaving them as-is.
But perhaps put a comment in the file saying that, so the next time we
run across them in a few years, we don't freak out and worry :)

thanks,

greg k-h

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

[Ard Biesheuvel]

On 1 December 2017 at 09:48, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
>> On 30 November 2017 at 17:10, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > 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@linux-foundation.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?
>> >
>>
>> These are the virtual mappings UEFI firmware regions, which must
>> remain in the same place across kexec reboots. So kexec tooling
>> consumes this information and passes it on to the incoming kernel in
>> some way.
>>
>> Note that these are not kernel addresses, so while I agree they should
>> not be world readable, they won't give you any clue as to where the
>> kernel itself is mapped.
>>
>> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
>> If so, I'll code up a patch.
>
> If these pointers are not "real", I recommend just leaving them as-is.

That's not what I said :-)

These are real pointers, and stuff will actually be mapped there
(although I am not intimately familiar with the way x86 does this, but
on ARM [which does not have these sysfs nodes in the first place],
these mappings are only live during the time a UEFI runtime service
call is in progress, and IIRC, work was underway to do the same for
x86). So while these values don't correlate with the placement of
kernel data structures, they could still be useful for an attacker to
figure out where exploitable firmware memory regions are located,
especially given that some of these may be mapped RWX.

> But perhaps put a comment in the file saying that, so the next time we
> run across them in a few years, we don't freak out and worry :)
>
> thanks,
>
> greg k-h

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

[Greg Kroah-Hartman]

On Fri, Dec 01, 2017 at 09:54:43AM +0000, Ard Biesheuvel wrote:
> On 1 December 2017 at 09:48, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > 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@linux-foundation.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?
> >> >
> >>
> >> These are the virtual mappings UEFI firmware regions, which must
> >> remain in the same place across kexec reboots. So kexec tooling
> >> consumes this information and passes it on to the incoming kernel in
> >> some way.
> >>
> >> Note that these are not kernel addresses, so while I agree they should
> >> not be world readable, they won't give you any clue as to where the
> >> kernel itself is mapped.
> >>
> >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> >> If so, I'll code up a patch.
> >
> > If these pointers are not "real", I recommend just leaving them as-is.
> 
> That's not what I said :-)
> 
> These are real pointers, and stuff will actually be mapped there
> (although I am not intimately familiar with the way x86 does this, but
> on ARM [which does not have these sysfs nodes in the first place],
> these mappings are only live during the time a UEFI runtime service
> call is in progress, and IIRC, work was underway to do the same for
> x86). So while these values don't correlate with the placement of
> kernel data structures, they could still be useful for an attacker to
> figure out where exploitable firmware memory regions are located,
> especially given that some of these may be mapped RWX.

Ah, ok, then yes, make that file readable from root only.

And isn't there a specific %p modifier you should use for a kernel
pointer.  I've lost the thread here for what should, or should not, be
done for kernel pointers these days based on the long email discussion.

thanks,

greg k-h

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

[Kees Cook]

On Fri, Dec 1, 2017 at 7:34 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> And isn't there a specific %p modifier you should use for a kernel
> pointer.  I've lost the thread here for what should, or should not, be
> done for kernel pointers these days based on the long email discussion.

Current implementation to bypass the hashing is %px. (Though perhaps
all %px usage should include a comment with a justification?)

-Kees

-- 
Kees Cook
Pixel Security

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

[Ard Biesheuvel]

On 1 December 2017 at 16:33, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 1, 2017 at 7:34 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>
>> And isn't there a specific %p modifier you should use for a kernel
>> pointer.  I've lost the thread here for what should, or should not, be
>> done for kernel pointers these days based on the long email discussion.
>
> Current implementation to bypass the hashing is %px. (Though perhaps
> all %px usage should include a comment with a justification?)
>

In this case, we're always dealing with u64 types regardless of the
pointer size and physical address size. So I am leaning towards
retaining the %llx, and only updating the sysfs node permissions.

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

[Matt Fleming]

(Cc'ing Dave since this is used for kexec on EFI)

On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> On 1 December 2017 at 09:48, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > 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@linux-foundation.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?
> >> >
> >>
> >> These are the virtual mappings UEFI firmware regions, which must
> >> remain in the same place across kexec reboots. So kexec tooling
> >> consumes this information and passes it on to the incoming kernel in
> >> some way.
> >>
> >> Note that these are not kernel addresses, so while I agree they should
> >> not be world readable, they won't give you any clue as to where the
> >> kernel itself is mapped.
> >>
> >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> >> If so, I'll code up a patch.
> >
> > If these pointers are not "real", I recommend just leaving them as-is.
> 
> That's not what I said :-)
> 
> These are real pointers, and stuff will actually be mapped there
> (although I am not intimately familiar with the way x86 does this, but
> on ARM [which does not have these sysfs nodes in the first place],
> these mappings are only live during the time a UEFI runtime service
> call is in progress, and IIRC, work was underway to do the same for
> x86). So while these values don't correlate with the placement of
> kernel data structures, they could still be useful for an attacker to
> figure out where exploitable firmware memory regions are located,
> especially given that some of these may be mapped RWX.

These are mappings of the EFI firmware's runtime regions, dynamically
allocated by the kernel starting at EFI_VA_START. Because we only get
one chance to tell the firmware where we placed its regions (via
SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
reboots use the same addresses.

So that's why they're exported to userspace through sysfs.

Like Ard said, these mappings are not mapped into the regular process
address space. Instead, they're only used while making EFI runtime
calls.

But this is still an information leak. And I think _ATTR(..0400) is
the right way to fix it. Dave, could you double-check my logic and
write a patch?

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

[Dave Young]

On 12/02/17 at 10:22pm, Matt Fleming wrote:
> (Cc'ing Dave since this is used for kexec on EFI)
> 
> On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> > On 1 December 2017 at 09:48, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> > >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> > >> <gregkh@linuxfoundation.org> wrote:
> > >> > 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@linux-foundation.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?
> > >> >
> > >>
> > >> These are the virtual mappings UEFI firmware regions, which must
> > >> remain in the same place across kexec reboots. So kexec tooling
> > >> consumes this information and passes it on to the incoming kernel in
> > >> some way.
> > >>
> > >> Note that these are not kernel addresses, so while I agree they should
> > >> not be world readable, they won't give you any clue as to where the
> > >> kernel itself is mapped.
> > >>
> > >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> > >> If so, I'll code up a patch.
> > >
> > > If these pointers are not "real", I recommend just leaving them as-is.
> > 
> > That's not what I said :-)
> > 
> > These are real pointers, and stuff will actually be mapped there
> > (although I am not intimately familiar with the way x86 does this, but
> > on ARM [which does not have these sysfs nodes in the first place],
> > these mappings are only live during the time a UEFI runtime service
> > call is in progress, and IIRC, work was underway to do the same for
> > x86). So while these values don't correlate with the placement of
> > kernel data structures, they could still be useful for an attacker to
> > figure out where exploitable firmware memory regions are located,
> > especially given that some of these may be mapped RWX.
> 
> These are mappings of the EFI firmware's runtime regions, dynamically
> allocated by the kernel starting at EFI_VA_START. Because we only get
> one chance to tell the firmware where we placed its regions (via
> SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
> reboots use the same addresses.
> 
> So that's why they're exported to userspace through sysfs.
> 
> Like Ard said, these mappings are not mapped into the regular process
> address space. Instead, they're only used while making EFI runtime
> calls.
> 
> But this is still an information leak. And I think _ATTR(..0400) is
> the right way to fix it. Dave, could you double-check my logic and
> write a patch?

Matt, thanks for ccing me, I will look into it.

Thanks
Dave

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

[Dave Young]

On 12/02/17 at 10:22pm, Matt Fleming wrote:
> (Cc'ing Dave since this is used for kexec on EFI)
> 
> On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> > On 1 December 2017 at 09:48, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> > >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> > >> <gregkh@linuxfoundation.org> wrote:
> > >> > 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@linux-foundation.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?
> > >> >
> > >>
> > >> These are the virtual mappings UEFI firmware regions, which must
> > >> remain in the same place across kexec reboots. So kexec tooling
> > >> consumes this information and passes it on to the incoming kernel in
> > >> some way.
> > >>
> > >> Note that these are not kernel addresses, so while I agree they should
> > >> not be world readable, they won't give you any clue as to where the
> > >> kernel itself is mapped.
> > >>
> > >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> > >> If so, I'll code up a patch.
> > >
> > > If these pointers are not "real", I recommend just leaving them as-is.
> > 
> > That's not what I said :-)
> > 
> > These are real pointers, and stuff will actually be mapped there
> > (although I am not intimately familiar with the way x86 does this, but
> > on ARM [which does not have these sysfs nodes in the first place],
> > these mappings are only live during the time a UEFI runtime service
> > call is in progress, and IIRC, work was underway to do the same for
> > x86). So while these values don't correlate with the placement of
> > kernel data structures, they could still be useful for an attacker to
> > figure out where exploitable firmware memory regions are located,
> > especially given that some of these may be mapped RWX.
> 
> These are mappings of the EFI firmware's runtime regions, dynamically
> allocated by the kernel starting at EFI_VA_START. Because we only get
> one chance to tell the firmware where we placed its regions (via
> SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
> reboots use the same addresses.
> 
> So that's why they're exported to userspace through sysfs.
> 
> Like Ard said, these mappings are not mapped into the regular process
> address space. Instead, they're only used while making EFI runtime
> calls.
> 
> But this is still an information leak. And I think _ATTR(..0400) is
> the right way to fix it. Dave, could you double-check my logic and
> write a patch?

I think 0400 is good enough for this issue.

Greg, would you like to agree add an extra macro like below?
If you have no objection I can send this in a formal patch:

---
 drivers/firmware/efi/runtime-map.c |   10 +++++-----
 include/linux/sysfs.h              |    5 +++++
 2 files changed, 10 insertions(+), 5 deletions(-)

--- linux-x86.orig/drivers/firmware/efi/runtime-map.c
+++ linux-x86/drivers/firmware/efi/runtime-map.c
@@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobj
 	return map_attr->show(entry, buf);
 }
 
-static struct map_attribute map_type_attr = __ATTR_RO(type);
-static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
-static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
-static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
-static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
+static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
+static struct map_attribute map_phys_addr_attr   = __ATTR_IRUSR(phys_addr);
+static struct map_attribute map_virt_addr_attr  = __ATTR_IRUSR(virt_addr);
+static struct map_attribute map_num_pages_attr  = __ATTR_IRUSR(num_pages);
+static struct map_attribute map_attribute_attr  = __ATTR_IRUSR(attribute);
 
 /*
  * These are default attributes that are added for every memmap entry.
--- linux-x86.orig/include/linux/sysfs.h
+++ linux-x86/include/linux/sysfs.h
@@ -112,6 +112,11 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
+#define __ATTR_IRUSR(_name) {						\
+	.attr   = { .name = __stringify(_name), .mode = S_IRUSR },	\
+	.show   = _name##_show,						\
+}
+
 #define __ATTR_RO(_name) {						\
 	.attr	= { .name = __stringify(_name), .mode = S_IRUGO },	\
 	.show	= _name##_show,						\

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

[Joe Perches]

On Mon, 2017-12-04 at 10:02 +0800, Dave Young wrote:
> I think 0400 is good enough for this issue.
> 
> Greg, would you like to agree add an extra macro like below?
[]
> -static struct map_attribute map_type_attr = __ATTR_RO(type);
> -static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> -static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> -static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> -static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> +static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
> +static struct map_attribute map_phys_addr_attr   = __ATTR_IRUSR(phys_addr);
> +static struct map_attribute map_virt_addr_attr  = __ATTR_IRUSR(virt_addr);
> +static struct map_attribute map_num_pages_attr  = __ATTR_IRUSR(num_pages);
> +static struct map_attribute map_attribute_attr  = __ATTR_IRUSR(attribute);
>  
>  /*
>   * These are default attributes that are added for every memmap entry.
> --- linux-x86.orig/include/linux/sysfs.h
> +++ linux-x86/include/linux/sysfs.h
> @@ -112,6 +112,11 @@ struct attribute_group {
>  	.store	= _store,						\
>  }
>  
> +#define __ATTR_IRUSR(_name) {					

I'd much prefer __ATTR_0400(_name)

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

[Dave Young]

On 12/03/17 at 06:33pm, Joe Perches wrote:
> On Mon, 2017-12-04 at 10:02 +0800, Dave Young wrote:
> > I think 0400 is good enough for this issue.
> > 
> > Greg, would you like to agree add an extra macro like below?
> []
> > -static struct map_attribute map_type_attr = __ATTR_RO(type);
> > -static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > -static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > -static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > -static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > +static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
> > +static struct map_attribute map_phys_addr_attr   = __ATTR_IRUSR(phys_addr);
> > +static struct map_attribute map_virt_addr_attr  = __ATTR_IRUSR(virt_addr);
> > +static struct map_attribute map_num_pages_attr  = __ATTR_IRUSR(num_pages);
> > +static struct map_attribute map_attribute_attr  = __ATTR_IRUSR(attribute);
> >  
> >  /*
> >   * These are default attributes that are added for every memmap entry.
> > --- linux-x86.orig/include/linux/sysfs.h
> > +++ linux-x86/include/linux/sysfs.h
> > @@ -112,6 +112,11 @@ struct attribute_group {
> >  	.store	= _store,						\
> >  }
> >  
> > +#define __ATTR_IRUSR(_name) {					
> 
> I'd much prefer __ATTR_0400(_name)
> 

I'm also fine with above, easier to get the meaning.

Thanks for the suggestion.
Dave

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

[Greg Kroah-Hartman]

On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> +#define __ATTR_IRUSR(_name) {						\
> +	.attr   = { .name = __stringify(_name), .mode = S_IRUSR },	\
> +	.show   = _name##_show,						\
> +}

Ick, no, as others, including Linus, have said, using IRUSER is a pain
in the ass to try to look up and remember what it is...

Just use __ATTR() please, it should be fine for what you need to do,
which is special-case a sysfs attribute.

thanks,

greg k-h

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

[Dave Young]

On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> > +#define __ATTR_IRUSR(_name) {						\
> > +	.attr   = { .name = __stringify(_name), .mode = S_IRUSR },	\
> > +	.show   = _name##_show,						\
> > +}
> 
> Ick, no, as others, including Linus, have said, using IRUSER is a pain
> in the ass to try to look up and remember what it is...
> 
> Just use __ATTR() please, it should be fine for what you need to do,
> which is special-case a sysfs attribute.

Hmm, I was hesitating to do that because it needs either long code
(over 80 chars) or some driver internal macros.

There is already same issue in dmi-sysfs.c, it uses an internal macro
DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
there might be more for such special cases. Maybe we can add some
comment in sysfs.h to mention this is for some special case?

I can do something similar as dmi sysfs code though.

> 
> thanks,
> 
> greg k-h

Thanks
Dave

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

[Greg Kroah-Hartman]

On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> > > +#define __ATTR_IRUSR(_name) {						\
> > > +	.attr   = { .name = __stringify(_name), .mode = S_IRUSR },	\
> > > +	.show   = _name##_show,						\
> > > +}
> > 
> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> > in the ass to try to look up and remember what it is...
> > 
> > Just use __ATTR() please, it should be fine for what you need to do,
> > which is special-case a sysfs attribute.
> 
> Hmm, I was hesitating to do that because it needs either long code
> (over 80 chars) or some driver internal macros.
> 
> There is already same issue in dmi-sysfs.c, it uses an internal macro
> DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
> there might be more for such special cases. Maybe we can add some
> comment in sysfs.h to mention this is for some special case?
> 
> I can do something similar as dmi sysfs code though.

Hm, let me look at this this afternoon when I get through some stable
patches, it shouldn't be that complex to need a whole new macro...

thanks,

greg k-h

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

[Ard Biesheuvel]

On 4 December 2017 at 09:34, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
>> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
>> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
>> > > +#define __ATTR_IRUSR(_name) {                                            \
>> > > + .attr   = { .name = __stringify(_name), .mode = S_IRUSR },      \
>> > > + .show   = _name##_show,                                         \
>> > > +}
>> >
>> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
>> > in the ass to try to look up and remember what it is...
>> >
>> > Just use __ATTR() please, it should be fine for what you need to do,
>> > which is special-case a sysfs attribute.
>>
>> Hmm, I was hesitating to do that because it needs either long code
>> (over 80 chars) or some driver internal macros.
>>
>> There is already same issue in dmi-sysfs.c, it uses an internal macro
>> DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
>> there might be more for such special cases. Maybe we can add some
>> comment in sysfs.h to mention this is for some special case?
>>
>> I can do something similar as dmi sysfs code though.
>
> Hm, let me look at this this afternoon when I get through some stable
> patches, it shouldn't be that complex to need a whole new macro...
>

But wasn't that the whole point? That there is a macro that does what
you don't want (__ATTR_RO) and none that does what you do want?

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

[Greg Kroah-Hartman]

On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
> On 4 December 2017 at 09:34, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> >> > > +#define __ATTR_IRUSR(_name) {                                            \
> >> > > + .attr   = { .name = __stringify(_name), .mode = S_IRUSR },      \
> >> > > + .show   = _name##_show,                                         \
> >> > > +}
> >> >
> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> >> > in the ass to try to look up and remember what it is...
> >> >
> >> > Just use __ATTR() please, it should be fine for what you need to do,
> >> > which is special-case a sysfs attribute.
> >>
> >> Hmm, I was hesitating to do that because it needs either long code
> >> (over 80 chars) or some driver internal macros.
> >>
> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
> >> DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
> >> there might be more for such special cases. Maybe we can add some
> >> comment in sysfs.h to mention this is for some special case?
> >>
> >> I can do something similar as dmi sysfs code though.
> >
> > Hm, let me look at this this afternoon when I get through some stable
> > patches, it shouldn't be that complex to need a whole new macro...
> >
> 
> But wasn't that the whole point? That there is a macro that does what
> you don't want (__ATTR_RO) and none that does what you do want?

my point is that __ATTR() should work for you as-is...

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

[Ard Biesheuvel]

On 4 December 2017 at 09:59, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
>> On 4 December 2017 at 09:34, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
>> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
>> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
>> >> > > +#define __ATTR_IRUSR(_name) {                                            \
>> >> > > + .attr   = { .name = __stringify(_name), .mode = S_IRUSR },      \
>> >> > > + .show   = _name##_show,                                         \
>> >> > > +}
>> >> >
>> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
>> >> > in the ass to try to look up and remember what it is...
>> >> >
>> >> > Just use __ATTR() please, it should be fine for what you need to do,
>> >> > which is special-case a sysfs attribute.
>> >>
>> >> Hmm, I was hesitating to do that because it needs either long code
>> >> (over 80 chars) or some driver internal macros.
>> >>
>> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
>> >> DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
>> >> there might be more for such special cases. Maybe we can add some
>> >> comment in sysfs.h to mention this is for some special case?
>> >>
>> >> I can do something similar as dmi sysfs code though.
>> >
>> > Hm, let me look at this this afternoon when I get through some stable
>> > patches, it shouldn't be that complex to need a whole new macro...
>> >
>>
>> But wasn't that the whole point? That there is a macro that does what
>> you don't want (__ATTR_RO) and none that does what you do want?
>
> my point is that __ATTR() should work for you as-is...

Well, not entirely.

Not sure if the runtime-map code is doing anything wrong here, but it defines

struct map_attribute {
    struct attribute attr;
    ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
};

and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
the .store member as well, which does not exists, and so it cannot be
used directly.

So we should either add a .store member that is always NULL, or we
should add our own

#define __ATTR_0400(_name) { \
.attr = { .name = __stringify(_name), .mode = 0400 }, \
.show = _name##_show, \
}

that does not set .store at all.

-- 
Ard.

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

[Greg Kroah-Hartman]

On Mon, Dec 04, 2017 at 10:03:19AM +0000, Ard Biesheuvel wrote:
> On 4 December 2017 at 09:59, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
> >> On 4 December 2017 at 09:34, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> >> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> >> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> >> >> > > +#define __ATTR_IRUSR(_name) {                                            \
> >> >> > > + .attr   = { .name = __stringify(_name), .mode = S_IRUSR },      \
> >> >> > > + .show   = _name##_show,                                         \
> >> >> > > +}
> >> >> >
> >> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> >> >> > in the ass to try to look up and remember what it is...
> >> >> >
> >> >> > Just use __ATTR() please, it should be fine for what you need to do,
> >> >> > which is special-case a sysfs attribute.
> >> >>
> >> >> Hmm, I was hesitating to do that because it needs either long code
> >> >> (over 80 chars) or some driver internal macros.
> >> >>
> >> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
> >> >> DMI_SYSFS_ATTR for 0400 attr.  I did not search all the kernel code,
> >> >> there might be more for such special cases. Maybe we can add some
> >> >> comment in sysfs.h to mention this is for some special case?
> >> >>
> >> >> I can do something similar as dmi sysfs code though.
> >> >
> >> > Hm, let me look at this this afternoon when I get through some stable
> >> > patches, it shouldn't be that complex to need a whole new macro...
> >> >
> >>
> >> But wasn't that the whole point? That there is a macro that does what
> >> you don't want (__ATTR_RO) and none that does what you do want?
> >
> > my point is that __ATTR() should work for you as-is...
> 
> Well, not entirely.
> 
> Not sure if the runtime-map code is doing anything wrong here, but it defines
> 
> struct map_attribute {
>     struct attribute attr;
>     ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
> };
> 
> and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> the .store member as well, which does not exists, and so it cannot be
> used directly.
> 
> So we should either add a .store member that is always NULL, or we
> should add our own

You should add a .store member that is always null, as you are getting
away with a nice hack by relying on that not being present :)

> #define __ATTR_0400(_name) { \
> .attr = { .name = __stringify(_name), .mode = 0400 }, \
> .show = _name##_show, \
> }
> 
> that does not set .store at all.

Creating a new macro for every different mode value you are wanting to
use seems a bit overkill, just use __ATTR() as is please.  You are
already using it in other efi files today:
	drivers/firmware/efi/efi.c
	drivers/firmware/efi/esrt.c
thanks,

greg k-h

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

[David Laight]

From: Ard Biesheuvel
> Sent: 04 December 2017 10:03
...
> and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> the .store member as well, which does not exists, and so it cannot be
> used directly.
> 
> So we should either add a .store member that is always NULL, or we
> should add our own
> 
> #define __ATTR_0400(_name) { \
> .attr = { .name = __stringify(_name), .mode = 0400 }, \
> .show = _name##_show, \
> }

What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
Even if the mode allowed write, writes wouldn't happen.

	David

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

[Greg Kroah-Hartman]

On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> From: Ard Biesheuvel
> > Sent: 04 December 2017 10:03
> ...
> > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > the .store member as well, which does not exists, and so it cannot be
> > used directly.
> > 
> > So we should either add a .store member that is always NULL, or we
> > should add our own
> > 
> > #define __ATTR_0400(_name) { \
> > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > .show = _name##_show, \
> > }
> 
> What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> Even if the mode allowed write, writes wouldn't happen.

Ah, that might work, could you convert the other users of __ATTR() in
the efi code to use it as well?

thanks,

greg k-h

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

[Dave Young]

On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > From: Ard Biesheuvel
> > > Sent: 04 December 2017 10:03
> > ...
> > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > the .store member as well, which does not exists, and so it cannot be
> > > used directly.
> > > 
> > > So we should either add a .store member that is always NULL, or we
> > > should add our own
> > > 
> > > #define __ATTR_0400(_name) { \
> > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > .show = _name##_show, \
> > > }
> > 
> > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > Even if the mode allowed write, writes wouldn't happen.
> 
> Ah, that might work, could you convert the other users of __ATTR() in
> the efi code to use it as well?

$ grep __ATTR * -RI
efi.c:			__ATTR(systab, 0400, systab_show, NULL);
efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
efi.c:	__ATTR_RO(fw_platform_size);
esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);

Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
not necessary.

And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.

I can do it but need confirm, Is this what you prefer?

Thanks
Dave

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

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > From: Ard Biesheuvel
> > > > Sent: 04 December 2017 10:03
> > > ...
> > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > the .store member as well, which does not exists, and so it cannot be
> > > > used directly.
> > > > 
> > > > So we should either add a .store member that is always NULL, or we
> > > > should add our own
> > > > 
> > > > #define __ATTR_0400(_name) { \
> > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > .show = _name##_show, \
> > > > }
> > > 
> > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > Even if the mode allowed write, writes wouldn't happen.
> > 
> > Ah, that might work, could you convert the other users of __ATTR() in
> > the efi code to use it as well?
> 
> $ grep __ATTR * -RI
> efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> efi.c:	__ATTR_RO(fw_platform_size);
> esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> 
> Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> not necessary.
> 
> And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> 
> I can do it but need confirm, Is this what you prefer?

Yes, how about the patch below, it builds for me, haven't done anything
other than that to test it :)

Also, what's with the multi-line sysfs file systab?  That's really not
allowed, can you please remove it?  Also the first check for !kobj and
!buf is funny, that can never happen.

Please turn all of those different values into different sysfs files.

thanks,

greg k-h


diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index f70febf680c3..c3eefa126e3b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
 	return str - buf;
 }
 
-static struct kobj_attribute efi_attr_systab =
-			__ATTR(systab, 0400, systab_show, NULL);
+static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
 
 #define EFI_FIELD(var) efi.var
 
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index bd7ed3c1148a..7aae2483fcb9 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
 };
 
 /* Generic ESRT Entry ("ESRE") support. */
-static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
+static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
 {
 	char *str = buf;
 
@@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
 	return str - buf;
 }
 
-static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
-	esre_fw_class_show, NULL);
+static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
 
 #define esre_attr_decl(name, size, fmt) \
-static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
+static ssize_t name##_show(struct esre_entry *entry, char *buf) \
 { \
 	return sprintf(buf, fmt "\n", \
 		       le##size##_to_cpu(entry->esre.esre1->name)); \
 } \
 \
-static struct esre_attribute esre_##name = __ATTR(name, 0400, \
-	esre_##name##_show, NULL)
+static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
 
 esre_attr_decl(fw_type, 32, "%u");
 esre_attr_decl(fw_version, 32, "%u");
@@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
 
 /* support for displaying ESRT fields at the top level */
 #define esrt_attr_decl(name, size, fmt) \
-static ssize_t esrt_##name##_show(struct kobject *kobj, \
+static ssize_t name##_show(struct kobject *kobj, \
 				  struct kobj_attribute *attr, char *buf)\
 { \
 	return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
 } \
 \
-static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
-	esrt_##name##_show, NULL)
+static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
 
 esrt_attr_decl(fw_resource_count, 32, "%u");
 esrt_attr_decl(fw_resource_count_max, 32, "%u");
diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
index 8e64b77aeac9..f377609ff141 100644
--- a/drivers/firmware/efi/runtime-map.c
+++ b/drivers/firmware/efi/runtime-map.c
@@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
 	return map_attr->show(entry, buf);
 }
 
-static struct map_attribute map_type_attr = __ATTR_RO(type);
-static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
-static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
-static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
-static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
+static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
+static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
+static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
+static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
+static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
 
 /*
  * These are default attributes that are added for every memmap entry.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e32dfe098e82..a886133ae15c 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -117,6 +117,11 @@ struct attribute_group {
 	.show	= _name##_show,						\
 }
 
+#define __ATTR_RO_MODE(_name, _mode) {					\
+	.attr	= { .name = __stringify(_name), .mode = _mode },	\
+	.show	= _name##_show,						\
+}
+
 #define __ATTR_WO(_name) {						\
 	.attr	= { .name = __stringify(_name), .mode = S_IWUSR },	\
 	.store	= _name##_store,					\

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

[Dave Young]

On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > From: Ard Biesheuvel
> > > > > Sent: 04 December 2017 10:03
> > > > ...
> > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > used directly.
> > > > > 
> > > > > So we should either add a .store member that is always NULL, or we
> > > > > should add our own
> > > > > 
> > > > > #define __ATTR_0400(_name) { \
> > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > .show = _name##_show, \
> > > > > }
> > > > 
> > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > Even if the mode allowed write, writes wouldn't happen.
> > > 
> > > Ah, that might work, could you convert the other users of __ATTR() in
> > > the efi code to use it as well?
> > 
> > $ grep __ATTR * -RI
> > efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > efi.c:	__ATTR_RO(fw_platform_size);
> > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > 
> > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> > not necessary.
> > 
> > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > 
> > I can do it but need confirm, Is this what you prefer?
> 
> Yes, how about the patch below, it builds for me, haven't done anything
> other than that to test it :)

Thanks! Let me do a kexec test and a boot test for esrt.

> 
> Also, what's with the multi-line sysfs file systab?  That's really not
> allowed, can you please remove it?  Also the first check for !kobj and
> !buf is funny, that can never happen.

I thought to do that, but later worried about it will break things:
http://lists.infradead.org/pipermail/kexec/2013-December/010759.html

I also thought to add code comment to avoid future expanding of this
file. Maybe we can do this now.

> 
> Please turn all of those different values into different sysfs files.
> 
> thanks,
> 
> greg k-h
> 
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index f70febf680c3..c3eefa126e3b 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
>  	return str - buf;
>  }
>  
> -static struct kobj_attribute efi_attr_systab =
> -			__ATTR(systab, 0400, systab_show, NULL);
> +static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
>  
>  #define EFI_FIELD(var) efi.var
>  
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index bd7ed3c1148a..7aae2483fcb9 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
>  };
>  
>  /* Generic ESRT Entry ("ESRE") support. */
> -static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> +static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
>  {
>  	char *str = buf;
>  
> @@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
>  	return str - buf;
>  }
>  
> -static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> -	esre_fw_class_show, NULL);
> +static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
>  
>  #define esre_attr_decl(name, size, fmt) \
> -static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
> +static ssize_t name##_show(struct esre_entry *entry, char *buf) \
>  { \
>  	return sprintf(buf, fmt "\n", \
>  		       le##size##_to_cpu(entry->esre.esre1->name)); \
>  } \
>  \
> -static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> -	esre_##name##_show, NULL)
> +static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
>  
>  esre_attr_decl(fw_type, 32, "%u");
>  esre_attr_decl(fw_version, 32, "%u");
> @@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
>  
>  /* support for displaying ESRT fields at the top level */
>  #define esrt_attr_decl(name, size, fmt) \
> -static ssize_t esrt_##name##_show(struct kobject *kobj, \
> +static ssize_t name##_show(struct kobject *kobj, \
>  				  struct kobj_attribute *attr, char *buf)\
>  { \
>  	return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
>  } \
>  \
> -static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> -	esrt_##name##_show, NULL)
> +static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
>  
>  esrt_attr_decl(fw_resource_count, 32, "%u");
>  esrt_attr_decl(fw_resource_count_max, 32, "%u");
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> index 8e64b77aeac9..f377609ff141 100644
> --- a/drivers/firmware/efi/runtime-map.c
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
>  	return map_attr->show(entry, buf);
>  }
>  
> -static struct map_attribute map_type_attr = __ATTR_RO(type);
> -static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> -static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> -static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> -static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> +static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
> +static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
> +static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
> +static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
> +static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
>  
>  /*
>   * These are default attributes that are added for every memmap entry.
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index e32dfe098e82..a886133ae15c 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -117,6 +117,11 @@ struct attribute_group {
>  	.show	= _name##_show,						\
>  }
>  
> +#define __ATTR_RO_MODE(_name, _mode) {					\
> +	.attr	= { .name = __stringify(_name), .mode = _mode },	\
> +	.show	= _name##_show,						\
> +}
> +
>  #define __ATTR_WO(_name) {						\
>  	.attr	= { .name = __stringify(_name), .mode = S_IWUSR },	\
>  	.store	= _name##_store,					\

Thanks
Dave

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

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > From: Ard Biesheuvel
> > > > > > Sent: 04 December 2017 10:03
> > > > > ...
> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > used directly.
> > > > > > 
> > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > should add our own
> > > > > > 
> > > > > > #define __ATTR_0400(_name) { \
> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > .show = _name##_show, \
> > > > > > }
> > > > > 
> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > 
> > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > the efi code to use it as well?
> > > 
> > > $ grep __ATTR * -RI
> > > efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > efi.c:	__ATTR_RO(fw_platform_size);
> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > > 
> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> > > not necessary.
> > > 
> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > 
> > > I can do it but need confirm, Is this what you prefer?
> > 
> > Yes, how about the patch below, it builds for me, haven't done anything
> > other than that to test it :)
> 
> Thanks! Let me do a kexec test and a boot test for esrt.
> 
> > 
> > Also, what's with the multi-line sysfs file systab?  That's really not
> > allowed, can you please remove it?  Also the first check for !kobj and
> > !buf is funny, that can never happen.
> 
> I thought to do that, but later worried about it will break things:
> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html

Heh, I guess I complained about this in the past :)

So what userspace tool uses it?

Are these values all exported through sysfs already?  If not, do that
first.

> I also thought to add code comment to avoid future expanding of this
> file. Maybe we can do this now.

Please do, but it should be a separate patch.

thanks,

greg k-h

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

[Ard Biesheuvel]

On 5 December 2017 at 08:52, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
>> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
>> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
>> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
>> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
>> > > > > From: Ard Biesheuvel
>> > > > > > Sent: 04 December 2017 10:03
>> > > > > ...
>> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
>> > > > > > the .store member as well, which does not exists, and so it cannot be
>> > > > > > used directly.
>> > > > > >
>> > > > > > So we should either add a .store member that is always NULL, or we
>> > > > > > should add our own
>> > > > > >
>> > > > > > #define __ATTR_0400(_name) { \
>> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
>> > > > > > .show = _name##_show, \
>> > > > > > }
>> > > > >
>> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
>> > > > > Even if the mode allowed write, writes wouldn't happen.
>> > > >
>> > > > Ah, that might work, could you convert the other users of __ATTR() in
>> > > > the efi code to use it as well?
>> > >
>> > > $ grep __ATTR * -RI
>> > > efi.c:                    __ATTR(systab, 0400, systab_show, NULL);
>> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
>> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>> > > efi.c:    __ATTR_RO(fw_platform_size);
>> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
>> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
>> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
>> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
>> > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
>> > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
>> > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
>> > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
>> > >
>> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
>> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
>> > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
>> > > not necessary.
>> > >
>> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
>> > >
>> > > I can do it but need confirm, Is this what you prefer?
>> >
>> > Yes, how about the patch below, it builds for me, haven't done anything
>> > other than that to test it :)
>>
>> Thanks! Let me do a kexec test and a boot test for esrt.
>>
>> >
>> > Also, what's with the multi-line sysfs file systab?  That's really not
>> > allowed, can you please remove it?  Also the first check for !kobj and
>> > !buf is funny, that can never happen.
>>
>> I thought to do that, but later worried about it will break things:
>> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
>
> Heh, I guess I complained about this in the past :)
>
> So what userspace tool uses it?
>

On x86, it is mostly tools that read DMI tables via /dev/mem, and use
/sys/firmware/efi/systab to locate them. dmidecode, lscpu, etc

That does mean we could investigate which entries are actually used,
and at least start removing the ones we don't need.

> Are these values all exported through sysfs already?  If not, do that
> first.
>
>> I also thought to add code comment to avoid future expanding of this
>> file. Maybe we can do this now.
>
> Please do, but it should be a separate patch.
>
> thanks,
>
> greg k-h

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

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 09:25:07AM +0000, Ard Biesheuvel wrote:
> On 5 December 2017 at 08:52, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> >> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> >> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> >> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> >> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> >> > > > > From: Ard Biesheuvel
> >> > > > > > Sent: 04 December 2017 10:03
> >> > > > > ...
> >> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> >> > > > > > the .store member as well, which does not exists, and so it cannot be
> >> > > > > > used directly.
> >> > > > > >
> >> > > > > > So we should either add a .store member that is always NULL, or we
> >> > > > > > should add our own
> >> > > > > >
> >> > > > > > #define __ATTR_0400(_name) { \
> >> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> >> > > > > > .show = _name##_show, \
> >> > > > > > }
> >> > > > >
> >> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> >> > > > > Even if the mode allowed write, writes wouldn't happen.
> >> > > >
> >> > > > Ah, that might work, could you convert the other users of __ATTR() in
> >> > > > the efi code to use it as well?
> >> > >
> >> > > $ grep __ATTR * -RI
> >> > > efi.c:                    __ATTR(systab, 0400, systab_show, NULL);
> >> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> >> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> >> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> >> > > efi.c:    __ATTR_RO(fw_platform_size);
> >> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> >> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> >> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> >> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> >> > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> >> > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> >> > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> >> > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> >> > >
> >> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> >> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> >> > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> >> > > not necessary.
> >> > >
> >> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> >> > >
> >> > > I can do it but need confirm, Is this what you prefer?
> >> >
> >> > Yes, how about the patch below, it builds for me, haven't done anything
> >> > other than that to test it :)
> >>
> >> Thanks! Let me do a kexec test and a boot test for esrt.
> >>
> >> >
> >> > Also, what's with the multi-line sysfs file systab?  That's really not
> >> > allowed, can you please remove it?  Also the first check for !kobj and
> >> > !buf is funny, that can never happen.
> >>
> >> I thought to do that, but later worried about it will break things:
> >> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
> >
> > Heh, I guess I complained about this in the past :)
> >
> > So what userspace tool uses it?
> >
> 
> On x86, it is mostly tools that read DMI tables via /dev/mem, and use
> /sys/firmware/efi/systab to locate them. dmidecode, lscpu, etc
> 
> That does mean we could investigate which entries are actually used,
> and at least start removing the ones we don't need.

That would be good to do.  Also, the data there is already covered by
other sysfs files, right?  If not, you should add that as individual
files first.

thanks,

greg k-h

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

[Dave Young]

On 12/05/17 at 09:52am, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> > On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > > From: Ard Biesheuvel
> > > > > > > Sent: 04 December 2017 10:03
> > > > > > ...
> > > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > > used directly.
> > > > > > > 
> > > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > > should add our own
> > > > > > > 
> > > > > > > #define __ATTR_0400(_name) { \
> > > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > > .show = _name##_show, \
> > > > > > > }
> > > > > > 
> > > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > > 
> > > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > > the efi code to use it as well?
> > > > 
> > > > $ grep __ATTR * -RI
> > > > efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> > > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > > efi.c:	__ATTR_RO(fw_platform_size);
> > > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > > > 
> > > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> > > > not necessary.
> > > > 
> > > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > > 
> > > > I can do it but need confirm, Is this what you prefer?
> > > 
> > > Yes, how about the patch below, it builds for me, haven't done anything
> > > other than that to test it :)
> > 
> > Thanks! Let me do a kexec test and a boot test for esrt.
> > 
> > > 
> > > Also, what's with the multi-line sysfs file systab?  That's really not
> > > allowed, can you please remove it?  Also the first check for !kobj and
> > > !buf is funny, that can never happen.
> > 
> > I thought to do that, but later worried about it will break things:
> > http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
> 
> Heh, I guess I complained about this in the past :)
> 
> So what userspace tool uses it?
> 
> Are these values all exported through sysfs already?  If not, do that
> first.

kexec-tools uses it, ie. SMBIOS/ACPI/ACPI20 which was exported before.
Later we added files out of systab though.

Since there are also other fields like MPS/BOOTINFO/UGA etc I'm not sure
if something else has been using it as well..

> 
> > I also thought to add code comment to avoid future expanding of this
> > file. Maybe we can do this now.
> 
> Please do, but it should be a separate patch.

Ok, will send it and cc you.

> 
> thanks,
> 
> greg k-h

Thanks
Dave

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

[Dave Young]

On 12/05/17 at 04:45pm, Dave Young wrote:
> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > From: Ard Biesheuvel
> > > > > > Sent: 04 December 2017 10:03
> > > > > ...
> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > used directly.
> > > > > > 
> > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > should add our own
> > > > > > 
> > > > > > #define __ATTR_0400(_name) { \
> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > .show = _name##_show, \
> > > > > > }
> > > > > 
> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > 
> > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > the efi code to use it as well?
> > > 
> > > $ grep __ATTR * -RI
> > > efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > efi.c:	__ATTR_RO(fw_platform_size);
> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > > 
> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> > > not necessary.
> > > 
> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > 
> > > I can do it but need confirm, Is this what you prefer?
> > 
> > Yes, how about the patch below, it builds for me, haven't done anything
> > other than that to test it :)
> 
> Thanks! Let me do a kexec test and a boot test for esrt.

It works for me. esrt part only did a boot test and cat/ls.

> 
> > 
> > Also, what's with the multi-line sysfs file systab?  That's really not
> > allowed, can you please remove it?  Also the first check for !kobj and
> > !buf is funny, that can never happen.
> 
> I thought to do that, but later worried about it will break things:
> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
> 
> I also thought to add code comment to avoid future expanding of this
> file. Maybe we can do this now.
> 
> > 
> > Please turn all of those different values into different sysfs files.
> > 
> > thanks,
> > 
> > greg k-h
> > 
> > 
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index f70febf680c3..c3eefa126e3b 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
> >  	return str - buf;
> >  }
> >  
> > -static struct kobj_attribute efi_attr_systab =
> > -			__ATTR(systab, 0400, systab_show, NULL);
> > +static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
> >  
> >  #define EFI_FIELD(var) efi.var
> >  
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index bd7ed3c1148a..7aae2483fcb9 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
> >  };
> >  
> >  /* Generic ESRT Entry ("ESRE") support. */
> > -static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> > +static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
> >  {
> >  	char *str = buf;
> >  
> > @@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> >  	return str - buf;
> >  }
> >  
> > -static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > -	esre_fw_class_show, NULL);
> > +static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
> >  
> >  #define esre_attr_decl(name, size, fmt) \
> > -static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
> > +static ssize_t name##_show(struct esre_entry *entry, char *buf) \
> >  { \
> >  	return sprintf(buf, fmt "\n", \
> >  		       le##size##_to_cpu(entry->esre.esre1->name)); \
> >  } \
> >  \
> > -static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > -	esre_##name##_show, NULL)
> > +static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
> >  
> >  esre_attr_decl(fw_type, 32, "%u");
> >  esre_attr_decl(fw_version, 32, "%u");
> > @@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
> >  
> >  /* support for displaying ESRT fields at the top level */
> >  #define esrt_attr_decl(name, size, fmt) \
> > -static ssize_t esrt_##name##_show(struct kobject *kobj, \
> > +static ssize_t name##_show(struct kobject *kobj, \
> >  				  struct kobj_attribute *attr, char *buf)\
> >  { \
> >  	return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
> >  } \
> >  \
> > -static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > -	esrt_##name##_show, NULL)
> > +static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
> >  
> >  esrt_attr_decl(fw_resource_count, 32, "%u");
> >  esrt_attr_decl(fw_resource_count_max, 32, "%u");
> > diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> > index 8e64b77aeac9..f377609ff141 100644
> > --- a/drivers/firmware/efi/runtime-map.c
> > +++ b/drivers/firmware/efi/runtime-map.c
> > @@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
> >  	return map_attr->show(entry, buf);
> >  }
> >  
> > -static struct map_attribute map_type_attr = __ATTR_RO(type);
> > -static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > -static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > -static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > -static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > +static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
> > +static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
> > +static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
> > +static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
> > +static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
> >  
> >  /*
> >   * These are default attributes that are added for every memmap entry.
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index e32dfe098e82..a886133ae15c 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -117,6 +117,11 @@ struct attribute_group {
> >  	.show	= _name##_show,						\
> >  }
> >  
> > +#define __ATTR_RO_MODE(_name, _mode) {					\
> > +	.attr	= { .name = __stringify(_name), .mode = _mode },	\
> > +	.show	= _name##_show,						\
> > +}
> > +
> >  #define __ATTR_WO(_name) {						\
> >  	.attr	= { .name = __stringify(_name), .mode = S_IWUSR },	\
> >  	.store	= _name##_store,					\
> 
> Thanks
> Dave

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

[Greg Kroah-Hartman]

On Tue, Dec 05, 2017 at 05:24:10PM +0800, Dave Young wrote:
> On 12/05/17 at 04:45pm, Dave Young wrote:
> > On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > > From: Ard Biesheuvel
> > > > > > > Sent: 04 December 2017 10:03
> > > > > > ...
> > > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > > used directly.
> > > > > > > 
> > > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > > should add our own
> > > > > > > 
> > > > > > > #define __ATTR_0400(_name) { \
> > > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > > .show = _name##_show, \
> > > > > > > }
> > > > > > 
> > > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > > 
> > > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > > the efi code to use it as well?
> > > > 
> > > > $ grep __ATTR * -RI
> > > > efi.c:			__ATTR(systab, 0400, systab_show, NULL);
> > > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > > efi.c:	__ATTR_RO(fw_platform_size);
> > > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > > runtime-map.c:static struct map_attribute map_phys_addr_attr   = __ATTR_RO(phys_addr);
> > > > runtime-map.c:static struct map_attribute map_virt_addr_attr  = __ATTR_RO(virt_addr);
> > > > runtime-map.c:static struct map_attribute map_num_pages_attr  = __ATTR_RO(num_pages);
> > > > runtime-map.c:static struct map_attribute map_attribute_attr  = __ATTR_RO(attribute);
> > > > 
> > > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > > drivers/firmware/dmi-sysfs.c can be updated.  But esrt.c __ATTR seems
> > > > not necessary.
> > > > 
> > > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > > 
> > > > I can do it but need confirm, Is this what you prefer?
> > > 
> > > Yes, how about the patch below, it builds for me, haven't done anything
> > > other than that to test it :)
> > 
> > Thanks! Let me do a kexec test and a boot test for esrt.
> 
> It works for me. esrt part only did a boot test and cat/ls.

Great, thanks for testing, I've sent it off now as a "real" patch.

greg k-h

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

[Linus Torvalds]

On Thu, Nov 30, 2017 at 12:10 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> 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?

So what I find distasteful here is how sysfs has these "helper" macros
that are clearly designed to over-share.

The __ATTR macro is a lot  more complicated to use than the
__ATTR_RO/WO/RW macros, but those macros end up giving everybody read
access (ok, not the WO one)

So honestly, I think the "helper" functions should be deprecated
simply because they basically encourage people to make everything
world-readable.

Which is why most of sysfs is world-readable, whether it makes sense or not.

It would have been better had they just taken the actual mode, I suspect.

(And it would be better yet if the code didn't use that disgusting
S_IRUGO, which pretty much everybody has to think about to figure out
it's 0444)

               Linus

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

[Greg Kroah-Hartman]

On Thu, Nov 30, 2017 at 06:17:47PM -0500, Linus Torvalds wrote:
> On Thu, Nov 30, 2017 at 12:10 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > 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?
> 
> So what I find distasteful here is how sysfs has these "helper" macros
> that are clearly designed to over-share.

That is by design :)

> The __ATTR macro is a lot  more complicated to use than the
> __ATTR_RO/WO/RW macros, but those macros end up giving everybody read
> access (ok, not the WO one)
> 
> So honestly, I think the "helper" functions should be deprecated
> simply because they basically encourage people to make everything
> world-readable.

Almost all information in sysfs is designed to be world-readable.  I
would argue that almost nothing there should be "root only", as sysfs is
not the place for trying to display "private" information at all.  It is
designed to show a representation of the kernel's internal state of
things (device structure, driver options, firmware objects, etc.)  All
of that information should be freely available to everyone (within the
namespace rules.)

To put root-only information in sysfs is not a good idea, which is why
those macros are there, to make it easy to do it right, and hard to
restrict information.  If you need restrictions, you shouldn't be using
sysfs, as it's almost always something for debugging, and that's what
debugfs is for.

thanks,

greg k-h