hash addresses printed with %p

GIT PULL Tobin C. Harding 9 participants 43 messages Nov 29, 2017 – Dec 5, 2017

Subsystems (2)

Driver core EFI

lore

Share & tools

lore

Tobin C. Harding

author

2017-11-29 04:59 UTC #0 lore raw

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 <[email protected]>

----------------------------------------------------------------
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(-)

Linus Torvalds

2017-11-29 19:22 UTC #1 lore raw

Quoted 2 lines · Tobin C. Harding

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

Linus Torvalds

2017-11-29 19:39 UTC #2 lore raw

Quoted 9 lines · Linus Torvalds

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.

Quoted 5 lines

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

Joe Perches

2017-11-29 20:54 UTC #3 lore raw

Quoted 31 lines · Linus Torvalds

Quoted 9 lines · Linus Torvalds
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.
Quoted 5 lines
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.

Quoted 6 lines

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

Linus Torvalds

2017-11-29 21:05 UTC #4 lore raw

Quoted 2 lines · Joe Perches

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

Kees Cook

2017-11-29 21:31 UTC #5 lore raw

In reply to Linus Torvalds

Quoted 24 lines · Linus Torvalds

Quoted 9 lines · Linus Torvalds
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.
Quoted 5 lines
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);

Quoted 6 lines

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".

Quoted 6 lines

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

Signature · 2 lines

Kees Cook
Pixel Security

Tobin C. Harding

author

2017-11-29 21:08 UTC #6 lore raw

In reply to Linus Torvalds

Quoted 5 lines · Linus Torvalds

Quoted 2 lines · Tobin C. Harding
git://github.com/tcharding/linux.git tags/printk-hash-pointer-4.15-rc2
Bah.

Sorry for creating extra work for you.

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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.

Linus Torvalds

2017-11-29 21:14 UTC #7 lore raw

Quoted 3 lines · Tobin C. Harding

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

Tobin C. Harding

author

2017-11-29 21:28 UTC #8 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Tobin C. Harding
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.

Quoted 6 lines

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 :)

Linus Torvalds

2017-11-29 21:36 UTC #9 lore raw

In reply to Linus Torvalds

Quoted 3 lines · Linus Torvalds

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

Greg Kroah-Hartman

maintainer

2017-11-30 16:32 UTC #10 lore raw

Quoted 19 lines · Linus Torvalds

Quoted 3 lines · Linus Torvalds
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.

Quoted 6 lines

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

drivers/firmware/efi/runtime-map.c +5 -5

index 8e64b77aeac9..09444964c8d7 100644
@@ -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.

Greg Kroah-Hartman

maintainer

2017-11-30 17:10 UTC #11 lore raw

Quoted 27 lines · Greg Kroah-Hartman

Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?

Quoted 8 lines

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

Ard Biesheuvel

2017-11-30 17:18 UTC #12 lore raw

Quoted 52 lines · Greg Kroah-Hartman

Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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

Quoted 4 lines

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.

Greg Kroah-Hartman

maintainer

2017-12-01 09:48 UTC #13 lore raw

Quoted 64 lines · Ard Biesheuvel

Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.

Quoted 6 lines

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

Ard Biesheuvel

2017-12-01 09:54 UTC #14 lore raw

Quoted 65 lines · Greg Kroah-Hartman

Quoted 64 lines · Ard Biesheuvel
Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.

Quoted 8 lines

Quoted 6 lines
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.

Quoted 6 lines

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

Greg Kroah-Hartman

maintainer

2017-12-01 15:34 UTC #15 lore raw

Quoted 78 lines · Ard Biesheuvel

Quoted 65 lines · Greg Kroah-Hartman
Quoted 64 lines · Ard Biesheuvel
Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
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 :-)

Quoted 9 lines

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

Kees Cook

2017-12-01 16:33 UTC #16 lore raw

Quoted 3 lines · Greg Kroah-Hartman

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

Signature · 2 lines

Kees Cook
Pixel Security

Ard Biesheuvel

2017-12-02 08:51 UTC #17 lore raw

Quoted 10 lines · Kees Cook

Quoted 3 lines · Greg Kroah-Hartman
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.

Matt Fleming

2017-12-02 22:22 UTC #18 lore raw

In reply to Ard Biesheuvel

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

Quoted 78 lines · Ard Biesheuvel

Quoted 65 lines · Greg Kroah-Hartman
Quoted 64 lines · Ard Biesheuvel
Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
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 :-)

Quoted 9 lines

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?

Dave Young

2017-12-03 01:15 UTC #19 lore raw

Quoted 99 lines · Matt Fleming

(Cc'ing Dave since this is used for kexec on EFI)
Quoted 78 lines · Ard Biesheuvel
Quoted 65 lines · Greg Kroah-Hartman
Quoted 64 lines · Ard Biesheuvel
Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
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 :-)
Quoted 9 lines
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.

Quoted 7 lines

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

Dave Young

2017-12-04 02:02 UTC #20 lore raw

In reply to Matt Fleming

Quoted 99 lines · Matt Fleming

(Cc'ing Dave since this is used for kexec on EFI)
Quoted 78 lines · Ard Biesheuvel
Quoted 65 lines · Greg Kroah-Hartman
Quoted 64 lines · Ard Biesheuvel
Quoted 52 lines · Greg Kroah-Hartman
Quoted 27 lines · Greg Kroah-Hartman
Quoted 19 lines · Linus Torvalds
Quoted 3 lines · Linus Torvalds
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.
Quoted 6 lines
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?
Quoted 8 lines
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
Quoted 4 lines
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.
Quoted 8 lines
Quoted 6 lines
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 :-)
Quoted 9 lines
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.

Quoted 7 lines

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(-)

drivers/firmware/efi/runtime-map.c +5 -5

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

include/linux/sysfs.h +5 -0

@@ -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,						\

Joe Perches

2017-12-04 02:33 UTC #21 lore raw

Quoted 3 lines · Dave Young

I think 0400 is good enough for this issue.
Greg, would you like to agree add an extra macro like below?

[]

Quoted 16 lines

drivers/firmware/efi/runtime-map.c +5 -5

-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.

include/linux/sysfs.h struct attribute_group { · line 115

Quoted 4 lines

include/linux/sysfs.h +1 -0

@@ -112,6 +112,11 @@ struct attribute_group {
 	.store	= _store,						\
 }
 
+#define __ATTR_IRUSR(_name) {

I'd much prefer __ATTR_0400(_name)

Dave Young

2017-12-04 02:39 UTC #22 lore raw

Quoted 24 lines · Joe Perches

Quoted 3 lines · Dave Young

I think 0400 is good enough for this issue.
Greg, would you like to agree add an extra macro like below?

[]

Quoted 19 lines

drivers/firmware/efi/runtime-map.c +5 -5

-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.

include/linux/sysfs.h +0 -0

@@ -112,6 +112,11 @@ struct attribute_group {
 	.store	= _store,						\
 }

Quoted 4 lines

include/linux/sysfs.h struct attribute_group { · line 115
Quoted 1 line
include/linux/sysfs.h +1 -0
+#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

Greg Kroah-Hartman

maintainer

2017-12-04 07:36 UTC #23 lore raw

In reply to Dave Young

include/linux/sysfs.h struct attribute_group { · line 118

Quoted 4 lines · Dave Young

include/linux/sysfs.h +4 -0

+#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

Dave Young

2017-12-04 09:29 UTC #24 lore raw

Quoted 11 lines · Greg Kroah-Hartman

include/linux/sysfs.h struct attribute_group { · line 118
Quoted 4 lines · Dave Young
include/linux/sysfs.h +4 -0
+#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.

Quoted 4 lines

thanks,
greg k-h

Thanks
Dave

Greg Kroah-Hartman

maintainer

2017-12-04 09:34 UTC #25 lore raw

Quoted 16 lines · Dave Young

Quoted 11 lines · Greg Kroah-Hartman
include/linux/sysfs.h struct attribute_group { · line 118
Quoted 4 lines · Dave Young
include/linux/sysfs.h +4 -0
+#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.

Quoted 6 lines

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

Ard Biesheuvel

2017-12-04 09:48 UTC #26 lore raw

Quoted 22 lines · Greg Kroah-Hartman

Quoted 21 lines · Dave Young
Quoted 11 lines · Greg Kroah-Hartman
include/linux/sysfs.h struct attribute_group { · line 118
Quoted 4 lines · Dave Young
include/linux/sysfs.h +4 -0
+#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?

Quoted 5 lines

Quoted 1 line
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?

Greg Kroah-Hartman

maintainer

2017-12-04 09:59 UTC #27 lore raw

Quoted 24 lines · Ard Biesheuvel

Quoted 22 lines · Greg Kroah-Hartman
Quoted 21 lines · Dave Young
Quoted 11 lines · Greg Kroah-Hartman
include/linux/sysfs.h struct attribute_group { · line 118
Quoted 4 lines · Dave Young
include/linux/sysfs.h +4 -0
+#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?

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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...

Ard Biesheuvel

2017-12-04 10:03 UTC #28 lore raw

Quoted 27 lines · Greg Kroah-Hartman

Quoted 26 lines · Ard Biesheuvel
Quoted 24 lines · Greg Kroah-Hartman
Quoted 22 lines · Dave Young
Quoted 11 lines · Greg Kroah-Hartman
include/linux/sysfs.h struct attribute_group { · line 118
Quoted 4 lines · Dave Young
include/linux/sysfs.h +4 -0
+#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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

Signature · 1 line

Ard.

Greg Kroah-Hartman

maintainer

2017-12-04 10:11 UTC #29 lore raw

Quoted 47 lines · Ard Biesheuvel

Quoted 27 lines · Greg Kroah-Hartman
Quoted 26 lines · Ard Biesheuvel
Quoted 24 lines · Greg Kroah-Hartman
Quoted 22 lines · Dave Young
Quoted 11 lines · Greg Kroah-Hartman
Review
Quoted 4 lines · Dave Young
(quoted diff) +4 -0
+#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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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);
};

Quoted 6 lines

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 :)

Quoted 6 lines

#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

David Laight

2017-12-04 12:50 UTC #30 lore raw

In reply to Ard Biesheuvel

From: Ard Biesheuvel

Quoted 1 line

Sent: 04 December 2017 10:03

...

Quoted 11 lines

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

Greg Kroah-Hartman

maintainer

2017-12-04 14:00 UTC #31 lore raw

Quoted 10 lines · David Laight

From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 7 lines
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

Quoted 7 lines

Quoted 4 lines
#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

Dave Young

2017-12-05 05:14 UTC #32 lore raw

Quoted 16 lines · Greg Kroah-Hartman

Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}

Quoted 5 lines

Quoted 2 lines
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

Greg Kroah-Hartman

maintainer

2017-12-05 08:09 UTC #33 lore raw

Quoted 38 lines · Dave Young

Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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);

Quoted 8 lines

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

drivers/firmware/efi/efi.c +1 -2

index f70febf680c3..c3eefa126e3b 100644
@@ -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

drivers/firmware/efi/esrt.c +6 -9

index bd7ed3c1148a..7aae2483fcb9 100644
@@ -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");

drivers/firmware/efi/runtime-map.c +5 -5

index 8e64b77aeac9..f377609ff141 100644
@@ -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.

include/linux/sysfs.h +5 -0

index e32dfe098e82..a886133ae15c 100644
@@ -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,					\

Dave Young

2017-12-05 08:45 UTC #34 lore raw

Quoted 44 lines · Greg Kroah-Hartman

Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 4 lines

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.

Quoted 101 lines

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

thanks,

greg k-h

drivers/firmware/efi/efi.c +1 -2

index f70febf680c3..c3eefa126e3b 100644
@@ -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

drivers/firmware/efi/esrt.c +6 -9

index bd7ed3c1148a..7aae2483fcb9 100644
@@ -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");

drivers/firmware/efi/runtime-map.c +5 -5

index 8e64b77aeac9..f377609ff141 100644
@@ -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.

include/linux/sysfs.h +0 -0

index e32dfe098e82..a886133ae15c 100644
@@ -117,6 +117,11 @@ struct attribute_group {
 	.show	= _name##_show,						\

include/linux/sysfs.h struct attribute_group { · line 127

Quoted 10 lines

include/linux/sysfs.h +5 -0

 }
 
+#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

Greg Kroah-Hartman

maintainer

2017-12-05 08:52 UTC #35 lore raw

Quoted 55 lines · Dave Young

Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
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.

Quoted 2 lines

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

Ard Biesheuvel

2017-12-05 09:25 UTC #36 lore raw

Quoted 60 lines · Greg Kroah-Hartman

Quoted 59 lines · Dave Young
Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 4 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 11 lines

Are these values all exported through sysfs already? If not, do that first.
Quoted 2 lines
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

Greg Kroah-Hartman

maintainer

2017-12-05 10:15 UTC #37 lore raw

Quoted 67 lines · Ard Biesheuvel

Quoted 65 lines · Greg Kroah-Hartman
Quoted 55 lines · Dave Young
Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
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 :)

Quoted 8 lines

Quoted 2 lines
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

Dave Young

2017-12-05 09:32 UTC #38 lore raw

In reply to Greg Kroah-Hartman

Quoted 63 lines · Greg Kroah-Hartman

Quoted 55 lines · Dave Young
Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 1 line
Quoted 6 lines
Quoted 3 lines
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

Quoted 6 lines

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..

Quoted 5 lines

Quoted 3 lines
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.

Quoted 4 lines

thanks,
greg k-h

Thanks
Dave

Dave Young

2017-12-05 09:24 UTC #39 lore raw

In reply to Dave Young

Quoted 45 lines · Dave Young

Quoted 44 lines · Greg Kroah-Hartman
Quoted 43 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 126 lines

Quoted 5 lines

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.

Quoted 101 lines

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

thanks,

greg k-h

drivers/firmware/efi/efi.c +1 -2

index f70febf680c3..c3eefa126e3b 100644
@@ -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

drivers/firmware/efi/esrt.c +6 -9

index bd7ed3c1148a..7aae2483fcb9 100644
@@ -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");

drivers/firmware/efi/runtime-map.c +5 -5

index 8e64b77aeac9..f377609ff141 100644
@@ -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.

include/linux/sysfs.h +0 -0

index e32dfe098e82..a886133ae15c 100644
@@ -117,6 +117,11 @@ struct attribute_group {
 	.show	= _name##_show,						\

include/linux/sysfs.h struct attribute_group { · line 127

Quoted 10 lines

include/linux/sysfs.h +5 -0

 }
 
+#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

Greg Kroah-Hartman

maintainer

2017-12-05 10:14 UTC #40 lore raw

Quoted 48 lines · Dave Young

Quoted 47 lines · Dave Young
Quoted 46 lines · Greg Kroah-Hartman
Quoted 45 lines · Dave Young
Quoted 16 lines · Greg Kroah-Hartman
Quoted 15 lines · David Laight
From: Ard Biesheuvel
Quoted 1 line
Sent: 04 December 2017 10:03
...
Quoted 11 lines
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, \
}
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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

Linus Torvalds

2017-11-30 23:17 UTC #41 lore raw

In reply to Greg Kroah-Hartman

Quoted 4 lines · Greg Kroah-Hartman

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

Greg Kroah-Hartman

maintainer

2017-12-01 09:47 UTC #42 lore raw

Quoted 9 lines · Linus Torvalds

Quoted 4 lines · Greg Kroah-Hartman
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 :)

Quoted 7 lines

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