On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> >> > <[email protected]> wrote:
> >> > >
> >> > > Not because %pK itself changed, but because the semantics of %p did.
> >> > > The baseline moved, and the "safe" version did not.
> >> >
> >> > Btw, that baseline for me is now that I can do
> >> >
> >> > ./scripts/leaking_addresses.pl | wc -l
> >> > 18
> >> >
> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> >> > the uevent keys).
> >> >
> >> > The remaining 12 are from the EFI runtime map files
> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> >> > that by default.
> >> >
> >> > I think the sysfs code makes it insanely too easy to make things
> >> > world-readable. You try to be careful, and mark things read-only etc,
> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> >> >
> >> > There seems to be no convenient model for kobjects having better
> >> > permissions. Greg?
> >>
> >> They can just use __ATTR() which lets you set the exact mode settings
> >> that are wanted.
> >>
> >> Something like the patch below, which breaks the build as the
> >> map_attributes are "odd", but you get the idea. The EFI developers can
> >> fix this up properly :)
> >>
> >> Note, this only accounts for 5 attributes, what is the whole list?
> >
> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> >
> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
> >
> > So changing it to use __ATTR() should fix this remaning leakage up.
> > That is if we even really need to export these values at all. What does
> > userspace do with them? Shouldn't they just be in debugfs instead?
> >
>
> These are the virtual mappings UEFI firmware regions, which must
> remain in the same place across kexec reboots. So kexec tooling
> consumes this information and passes it on to the incoming kernel in
> some way.
>
> Note that these are not kernel addresses, so while I agree they should
> not be world readable, they won't give you any clue as to where the
> kernel itself is mapped.
>
> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> If so, I'll code up a patch.
If these pointers are not "real", I recommend just leaving them as-is.
But perhaps put a comment in the file saying that, so the next time we
run across them in a few years, we don't freak out and worry :)
thanks,
greg k-h
On 1 December 2017 at 09:48, Greg Kroah-Hartman
<[email protected]> wrote:
> On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
>> On 30 November 2017 at 17:10, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
>> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
>> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
>> >> > <[email protected]> wrote:
>> >> > >
>> >> > > Not because %pK itself changed, but because the semantics of %p did.
>> >> > > The baseline moved, and the "safe" version did not.
>> >> >
>> >> > Btw, that baseline for me is now that I can do
>> >> >
>> >> > ./scripts/leaking_addresses.pl | wc -l
>> >> > 18
>> >> >
>> >> > and of those 18 hits, six are false positives (looks like bitmaps in
>> >> > the uevent keys).
>> >> >
>> >> > The remaining 12 are from the EFI runtime map files
>> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
>> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
>> >> > that by default.
>> >> >
>> >> > I think the sysfs code makes it insanely too easy to make things
>> >> > world-readable. You try to be careful, and mark things read-only etc,
>> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
>> >> >
>> >> > There seems to be no convenient model for kobjects having better
>> >> > permissions. Greg?
>> >>
>> >> They can just use __ATTR() which lets you set the exact mode settings
>> >> that are wanted.
>> >>
>> >> Something like the patch below, which breaks the build as the
>> >> map_attributes are "odd", but you get the idea. The EFI developers can
>> >> fix this up properly :)
>> >>
>> >> Note, this only accounts for 5 attributes, what is the whole list?
>> >
>> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
>> >
>> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
>> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
>> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
>> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
>> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
>> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
>> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
>> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
>> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
>> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
>> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
>> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
>> >
>> > So changing it to use __ATTR() should fix this remaning leakage up.
>> > That is if we even really need to export these values at all. What does
>> > userspace do with them? Shouldn't they just be in debugfs instead?
>> >
>>
>> These are the virtual mappings UEFI firmware regions, which must
>> remain in the same place across kexec reboots. So kexec tooling
>> consumes this information and passes it on to the incoming kernel in
>> some way.
>>
>> Note that these are not kernel addresses, so while I agree they should
>> not be world readable, they won't give you any clue as to where the
>> kernel itself is mapped.
>>
>> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
>> If so, I'll code up a patch.
>
> If these pointers are not "real", I recommend just leaving them as-is.
That's not what I said :-)
These are real pointers, and stuff will actually be mapped there
(although I am not intimately familiar with the way x86 does this, but
on ARM [which does not have these sysfs nodes in the first place],
these mappings are only live during the time a UEFI runtime service
call is in progress, and IIRC, work was underway to do the same for
x86). So while these values don't correlate with the placement of
kernel data structures, they could still be useful for an attacker to
figure out where exploitable firmware memory regions are located,
especially given that some of these may be mapped RWX.
> But perhaps put a comment in the file saying that, so the next time we
> run across them in a few years, we don't freak out and worry :)
>
> thanks,
>
> greg k-h
On Fri, Dec 01, 2017 at 09:54:43AM +0000, Ard Biesheuvel wrote:
> On 1 December 2017 at 09:48, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
> >> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> >> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> >> >> > <[email protected]> wrote:
> >> >> > >
> >> >> > > Not because %pK itself changed, but because the semantics of %p did.
> >> >> > > The baseline moved, and the "safe" version did not.
> >> >> >
> >> >> > Btw, that baseline for me is now that I can do
> >> >> >
> >> >> > ./scripts/leaking_addresses.pl | wc -l
> >> >> > 18
> >> >> >
> >> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> >> >> > the uevent keys).
> >> >> >
> >> >> > The remaining 12 are from the EFI runtime map files
> >> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> >> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> >> >> > that by default.
> >> >> >
> >> >> > I think the sysfs code makes it insanely too easy to make things
> >> >> > world-readable. You try to be careful, and mark things read-only etc,
> >> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> >> >> >
> >> >> > There seems to be no convenient model for kobjects having better
> >> >> > permissions. Greg?
> >> >>
> >> >> They can just use __ATTR() which lets you set the exact mode settings
> >> >> that are wanted.
> >> >>
> >> >> Something like the patch below, which breaks the build as the
> >> >> map_attributes are "odd", but you get the idea. The EFI developers can
> >> >> fix this up properly :)
> >> >>
> >> >> Note, this only accounts for 5 attributes, what is the whole list?
> >> >
> >> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> >> >
> >> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
> >> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
> >> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
> >> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
> >> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
> >> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
> >> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
> >> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
> >> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
> >> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
> >> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
> >> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
> >> >
> >> > So changing it to use __ATTR() should fix this remaning leakage up.
> >> > That is if we even really need to export these values at all. What does
> >> > userspace do with them? Shouldn't they just be in debugfs instead?
> >> >
> >>
> >> These are the virtual mappings UEFI firmware regions, which must
> >> remain in the same place across kexec reboots. So kexec tooling
> >> consumes this information and passes it on to the incoming kernel in
> >> some way.
> >>
> >> Note that these are not kernel addresses, so while I agree they should
> >> not be world readable, they won't give you any clue as to where the
> >> kernel itself is mapped.
> >>
> >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> >> If so, I'll code up a patch.
> >
> > If these pointers are not "real", I recommend just leaving them as-is.
>
> That's not what I said :-)
>
> These are real pointers, and stuff will actually be mapped there
> (although I am not intimately familiar with the way x86 does this, but
> on ARM [which does not have these sysfs nodes in the first place],
> these mappings are only live during the time a UEFI runtime service
> call is in progress, and IIRC, work was underway to do the same for
> x86). So while these values don't correlate with the placement of
> kernel data structures, they could still be useful for an attacker to
> figure out where exploitable firmware memory regions are located,
> especially given that some of these may be mapped RWX.
Ah, ok, then yes, make that file readable from root only.
And isn't there a specific %p modifier you should use for a kernel
pointer. I've lost the thread here for what should, or should not, be
done for kernel pointers these days based on the long email discussion.
thanks,
greg k-h
On Fri, Dec 1, 2017 at 7:34 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> And isn't there a specific %p modifier you should use for a kernel
> pointer. I've lost the thread here for what should, or should not, be
> done for kernel pointers these days based on the long email discussion.
Current implementation to bypass the hashing is %px. (Though perhaps
all %px usage should include a comment with a justification?)
-Kees
--
Kees Cook
Pixel Security
On 1 December 2017 at 16:33, Kees Cook <[email protected]> wrote:
> On Fri, Dec 1, 2017 at 7:34 AM, Greg Kroah-Hartman
> <[email protected]> wrote:
>
>> And isn't there a specific %p modifier you should use for a kernel
>> pointer. I've lost the thread here for what should, or should not, be
>> done for kernel pointers these days based on the long email discussion.
>
> Current implementation to bypass the hashing is %px. (Though perhaps
> all %px usage should include a comment with a justification?)
>
In this case, we're always dealing with u64 types regardless of the
pointer size and physical address size. So I am leaning towards
retaining the %llx, and only updating the sysfs node permissions.
(Cc'ing Dave since this is used for kexec on EFI)
On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> On 1 December 2017 at 09:48, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
> >> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> >> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> >> >> > <[email protected]> wrote:
> >> >> > >
> >> >> > > Not because %pK itself changed, but because the semantics of %p did.
> >> >> > > The baseline moved, and the "safe" version did not.
> >> >> >
> >> >> > Btw, that baseline for me is now that I can do
> >> >> >
> >> >> > ./scripts/leaking_addresses.pl | wc -l
> >> >> > 18
> >> >> >
> >> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> >> >> > the uevent keys).
> >> >> >
> >> >> > The remaining 12 are from the EFI runtime map files
> >> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> >> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> >> >> > that by default.
> >> >> >
> >> >> > I think the sysfs code makes it insanely too easy to make things
> >> >> > world-readable. You try to be careful, and mark things read-only etc,
> >> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> >> >> >
> >> >> > There seems to be no convenient model for kobjects having better
> >> >> > permissions. Greg?
> >> >>
> >> >> They can just use __ATTR() which lets you set the exact mode settings
> >> >> that are wanted.
> >> >>
> >> >> Something like the patch below, which breaks the build as the
> >> >> map_attributes are "odd", but you get the idea. The EFI developers can
> >> >> fix this up properly :)
> >> >>
> >> >> Note, this only accounts for 5 attributes, what is the whole list?
> >> >
> >> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> >> >
> >> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
> >> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
> >> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
> >> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
> >> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
> >> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
> >> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
> >> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
> >> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
> >> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
> >> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
> >> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
> >> >
> >> > So changing it to use __ATTR() should fix this remaning leakage up.
> >> > That is if we even really need to export these values at all. What does
> >> > userspace do with them? Shouldn't they just be in debugfs instead?
> >> >
> >>
> >> These are the virtual mappings UEFI firmware regions, which must
> >> remain in the same place across kexec reboots. So kexec tooling
> >> consumes this information and passes it on to the incoming kernel in
> >> some way.
> >>
> >> Note that these are not kernel addresses, so while I agree they should
> >> not be world readable, they won't give you any clue as to where the
> >> kernel itself is mapped.
> >>
> >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> >> If so, I'll code up a patch.
> >
> > If these pointers are not "real", I recommend just leaving them as-is.
>
> That's not what I said :-)
>
> These are real pointers, and stuff will actually be mapped there
> (although I am not intimately familiar with the way x86 does this, but
> on ARM [which does not have these sysfs nodes in the first place],
> these mappings are only live during the time a UEFI runtime service
> call is in progress, and IIRC, work was underway to do the same for
> x86). So while these values don't correlate with the placement of
> kernel data structures, they could still be useful for an attacker to
> figure out where exploitable firmware memory regions are located,
> especially given that some of these may be mapped RWX.
These are mappings of the EFI firmware's runtime regions, dynamically
allocated by the kernel starting at EFI_VA_START. Because we only get
one chance to tell the firmware where we placed its regions (via
SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
reboots use the same addresses.
So that's why they're exported to userspace through sysfs.
Like Ard said, these mappings are not mapped into the regular process
address space. Instead, they're only used while making EFI runtime
calls.
But this is still an information leak. And I think _ATTR(..0400) is
the right way to fix it. Dave, could you double-check my logic and
write a patch?
On 12/02/17 at 10:22pm, Matt Fleming wrote:
> (Cc'ing Dave since this is used for kexec on EFI)
>
> On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> > On 1 December 2017 at 09:48, Greg Kroah-Hartman
> > <[email protected]> wrote:
> > > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> > >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> > >> <[email protected]> wrote:
> > >> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
> > >> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> > >> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> > >> >> > <[email protected]> wrote:
> > >> >> > >
> > >> >> > > Not because %pK itself changed, but because the semantics of %p did.
> > >> >> > > The baseline moved, and the "safe" version did not.
> > >> >> >
> > >> >> > Btw, that baseline for me is now that I can do
> > >> >> >
> > >> >> > ./scripts/leaking_addresses.pl | wc -l
> > >> >> > 18
> > >> >> >
> > >> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> > >> >> > the uevent keys).
> > >> >> >
> > >> >> > The remaining 12 are from the EFI runtime map files
> > >> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> > >> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> > >> >> > that by default.
> > >> >> >
> > >> >> > I think the sysfs code makes it insanely too easy to make things
> > >> >> > world-readable. You try to be careful, and mark things read-only etc,
> > >> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> > >> >> >
> > >> >> > There seems to be no convenient model for kobjects having better
> > >> >> > permissions. Greg?
> > >> >>
> > >> >> They can just use __ATTR() which lets you set the exact mode settings
> > >> >> that are wanted.
> > >> >>
> > >> >> Something like the patch below, which breaks the build as the
> > >> >> map_attributes are "odd", but you get the idea. The EFI developers can
> > >> >> fix this up properly :)
> > >> >>
> > >> >> Note, this only accounts for 5 attributes, what is the whole list?
> > >> >
> > >> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> > >> >
> > >> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
> > >> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
> > >> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
> > >> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
> > >> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
> > >> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
> > >> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
> > >> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
> > >> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
> > >> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
> > >> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
> > >> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
> > >> >
> > >> > So changing it to use __ATTR() should fix this remaning leakage up.
> > >> > That is if we even really need to export these values at all. What does
> > >> > userspace do with them? Shouldn't they just be in debugfs instead?
> > >> >
> > >>
> > >> These are the virtual mappings UEFI firmware regions, which must
> > >> remain in the same place across kexec reboots. So kexec tooling
> > >> consumes this information and passes it on to the incoming kernel in
> > >> some way.
> > >>
> > >> Note that these are not kernel addresses, so while I agree they should
> > >> not be world readable, they won't give you any clue as to where the
> > >> kernel itself is mapped.
> > >>
> > >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> > >> If so, I'll code up a patch.
> > >
> > > If these pointers are not "real", I recommend just leaving them as-is.
> >
> > That's not what I said :-)
> >
> > These are real pointers, and stuff will actually be mapped there
> > (although I am not intimately familiar with the way x86 does this, but
> > on ARM [which does not have these sysfs nodes in the first place],
> > these mappings are only live during the time a UEFI runtime service
> > call is in progress, and IIRC, work was underway to do the same for
> > x86). So while these values don't correlate with the placement of
> > kernel data structures, they could still be useful for an attacker to
> > figure out where exploitable firmware memory regions are located,
> > especially given that some of these may be mapped RWX.
>
> These are mappings of the EFI firmware's runtime regions, dynamically
> allocated by the kernel starting at EFI_VA_START. Because we only get
> one chance to tell the firmware where we placed its regions (via
> SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
> reboots use the same addresses.
>
> So that's why they're exported to userspace through sysfs.
>
> Like Ard said, these mappings are not mapped into the regular process
> address space. Instead, they're only used while making EFI runtime
> calls.
>
> But this is still an information leak. And I think _ATTR(..0400) is
> the right way to fix it. Dave, could you double-check my logic and
> write a patch?
Matt, thanks for ccing me, I will look into it.
Thanks
Dave
On 12/02/17 at 10:22pm, Matt Fleming wrote:
> (Cc'ing Dave since this is used for kexec on EFI)
>
> On Fri, 01 Dec, at 09:54:43AM, Ard Biesheuvel wrote:
> > On 1 December 2017 at 09:48, Greg Kroah-Hartman
> > <[email protected]> wrote:
> > > On Thu, Nov 30, 2017 at 05:18:42PM +0000, Ard Biesheuvel wrote:
> > >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> > >> <[email protected]> wrote:
> > >> > On Thu, Nov 30, 2017 at 04:32:35PM +0000, Greg Kroah-Hartman wrote:
> > >> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> > >> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> > >> >> > <[email protected]> wrote:
> > >> >> > >
> > >> >> > > Not because %pK itself changed, but because the semantics of %p did.
> > >> >> > > The baseline moved, and the "safe" version did not.
> > >> >> >
> > >> >> > Btw, that baseline for me is now that I can do
> > >> >> >
> > >> >> > ./scripts/leaking_addresses.pl | wc -l
> > >> >> > 18
> > >> >> >
> > >> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> > >> >> > the uevent keys).
> > >> >> >
> > >> >> > The remaining 12 are from the EFI runtime map files
> > >> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> > >> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> > >> >> > that by default.
> > >> >> >
> > >> >> > I think the sysfs code makes it insanely too easy to make things
> > >> >> > world-readable. You try to be careful, and mark things read-only etc,
> > >> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> > >> >> >
> > >> >> > There seems to be no convenient model for kobjects having better
> > >> >> > permissions. Greg?
> > >> >>
> > >> >> They can just use __ATTR() which lets you set the exact mode settings
> > >> >> that are wanted.
> > >> >>
> > >> >> Something like the patch below, which breaks the build as the
> > >> >> map_attributes are "odd", but you get the idea. The EFI developers can
> > >> >> fix this up properly :)
> > >> >>
> > >> >> Note, this only accounts for 5 attributes, what is the whole list?
> > >> >
> > >> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> > >> >
> > >> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffffffeea6ea000
> > >> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffffffeee88b000
> > >> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffffffefea00000
> > >> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffffffed9c00000
> > >> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffffffefee00000
> > >> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffffffedba4e000
> > >> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffffffeee2de000
> > >> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffffffeeea00000
> > >> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffffffefec00000
> > >> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffffffed9c60000
> > >> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffffffeff000000
> > >> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffffffedb9c9000
> > >> >
> > >> > So changing it to use __ATTR() should fix this remaning leakage up.
> > >> > That is if we even really need to export these values at all. What does
> > >> > userspace do with them? Shouldn't they just be in debugfs instead?
> > >> >
> > >>
> > >> These are the virtual mappings UEFI firmware regions, which must
> > >> remain in the same place across kexec reboots. So kexec tooling
> > >> consumes this information and passes it on to the incoming kernel in
> > >> some way.
> > >>
> > >> Note that these are not kernel addresses, so while I agree they should
> > >> not be world readable, they won't give you any clue as to where the
> > >> kernel itself is mapped.
> > >>
> > >> So the recommendation is to switch to __ATTR( ... 0400 ... ) instead?
> > >> If so, I'll code up a patch.
> > >
> > > If these pointers are not "real", I recommend just leaving them as-is.
> >
> > That's not what I said :-)
> >
> > These are real pointers, and stuff will actually be mapped there
> > (although I am not intimately familiar with the way x86 does this, but
> > on ARM [which does not have these sysfs nodes in the first place],
> > these mappings are only live during the time a UEFI runtime service
> > call is in progress, and IIRC, work was underway to do the same for
> > x86). So while these values don't correlate with the placement of
> > kernel data structures, they could still be useful for an attacker to
> > figure out where exploitable firmware memory regions are located,
> > especially given that some of these may be mapped RWX.
>
> These are mappings of the EFI firmware's runtime regions, dynamically
> allocated by the kernel starting at EFI_VA_START. Because we only get
> one chance to tell the firmware where we placed its regions (via
> SetVirtualAddressMap()) we have to guarantee that any subsequent kexec
> reboots use the same addresses.
>
> So that's why they're exported to userspace through sysfs.
>
> Like Ard said, these mappings are not mapped into the regular process
> address space. Instead, they're only used while making EFI runtime
> calls.
>
> But this is still an information leak. And I think _ATTR(..0400) is
> the right way to fix it. Dave, could you double-check my logic and
> write a patch?
I think 0400 is good enough for this issue.
Greg, would you like to agree add an extra macro like below?
If you have no objection I can send this in a formal patch:
---
drivers/firmware/efi/runtime-map.c | 10 +++++-----
include/linux/sysfs.h | 5 +++++
2 files changed, 10 insertions(+), 5 deletions(-)
--- linux-x86.orig/drivers/firmware/efi/runtime-map.c
+++ linux-x86/drivers/firmware/efi/runtime-map.c
@@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobj
return map_attr->show(entry, buf);
}
-static struct map_attribute map_type_attr = __ATTR_RO(type);
-static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
-static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
-static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
-static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
+static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
+static struct map_attribute map_phys_addr_attr = __ATTR_IRUSR(phys_addr);
+static struct map_attribute map_virt_addr_attr = __ATTR_IRUSR(virt_addr);
+static struct map_attribute map_num_pages_attr = __ATTR_IRUSR(num_pages);
+static struct map_attribute map_attribute_attr = __ATTR_IRUSR(attribute);
/*
* These are default attributes that are added for every memmap entry.
--- linux-x86.orig/include/linux/sysfs.h
+++ linux-x86/include/linux/sysfs.h
@@ -112,6 +112,11 @@ struct attribute_group {
.store = _store, \
}
+#define __ATTR_IRUSR(_name) { \
+ .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
+ .show = _name##_show, \
+}
+
#define __ATTR_RO(_name) { \
.attr = { .name = __stringify(_name), .mode = S_IRUGO }, \
.show = _name##_show, \
On Mon, 2017-12-04 at 10:02 +0800, Dave Young wrote:
> I think 0400 is good enough for this issue.
>
> Greg, would you like to agree add an extra macro like below?
[]
> -static struct map_attribute map_type_attr = __ATTR_RO(type);
> -static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> -static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> -static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> -static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> +static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
> +static struct map_attribute map_phys_addr_attr = __ATTR_IRUSR(phys_addr);
> +static struct map_attribute map_virt_addr_attr = __ATTR_IRUSR(virt_addr);
> +static struct map_attribute map_num_pages_attr = __ATTR_IRUSR(num_pages);
> +static struct map_attribute map_attribute_attr = __ATTR_IRUSR(attribute);
>
> /*
> * These are default attributes that are added for every memmap entry.
> --- linux-x86.orig/include/linux/sysfs.h
> +++ linux-x86/include/linux/sysfs.h
> @@ -112,6 +112,11 @@ struct attribute_group {
> .store = _store, \
> }
>
> +#define __ATTR_IRUSR(_name) {
I'd much prefer __ATTR_0400(_name)
On 12/03/17 at 06:33pm, Joe Perches wrote:
> On Mon, 2017-12-04 at 10:02 +0800, Dave Young wrote:
> > I think 0400 is good enough for this issue.
> >
> > Greg, would you like to agree add an extra macro like below?
> []
> > -static struct map_attribute map_type_attr = __ATTR_RO(type);
> > -static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > -static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > -static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > -static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > +static struct map_attribute map_type_attr = __ATTR_IRUSR(type);
> > +static struct map_attribute map_phys_addr_attr = __ATTR_IRUSR(phys_addr);
> > +static struct map_attribute map_virt_addr_attr = __ATTR_IRUSR(virt_addr);
> > +static struct map_attribute map_num_pages_attr = __ATTR_IRUSR(num_pages);
> > +static struct map_attribute map_attribute_attr = __ATTR_IRUSR(attribute);
> >
> > /*
> > * These are default attributes that are added for every memmap entry.
> > --- linux-x86.orig/include/linux/sysfs.h
> > +++ linux-x86/include/linux/sysfs.h
> > @@ -112,6 +112,11 @@ struct attribute_group {
> > .store = _store, \
> > }
> >
> > +#define __ATTR_IRUSR(_name) {
>
> I'd much prefer __ATTR_0400(_name)
>
I'm also fine with above, easier to get the meaning.
Thanks for the suggestion.
Dave
On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> +#define __ATTR_IRUSR(_name) { \
> + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
> + .show = _name##_show, \
> +}
Ick, no, as others, including Linus, have said, using IRUSER is a pain
in the ass to try to look up and remember what it is...
Just use __ATTR() please, it should be fine for what you need to do,
which is special-case a sysfs attribute.
thanks,
greg k-h
On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> > +#define __ATTR_IRUSR(_name) { \
> > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
> > + .show = _name##_show, \
> > +}
>
> Ick, no, as others, including Linus, have said, using IRUSER is a pain
> in the ass to try to look up and remember what it is...
>
> Just use __ATTR() please, it should be fine for what you need to do,
> which is special-case a sysfs attribute.
Hmm, I was hesitating to do that because it needs either long code
(over 80 chars) or some driver internal macros.
There is already same issue in dmi-sysfs.c, it uses an internal macro
DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
there might be more for such special cases. Maybe we can add some
comment in sysfs.h to mention this is for some special case?
I can do something similar as dmi sysfs code though.
>
> thanks,
>
> greg k-h
Thanks
Dave
On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> > > +#define __ATTR_IRUSR(_name) { \
> > > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
> > > + .show = _name##_show, \
> > > +}
> >
> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> > in the ass to try to look up and remember what it is...
> >
> > Just use __ATTR() please, it should be fine for what you need to do,
> > which is special-case a sysfs attribute.
>
> Hmm, I was hesitating to do that because it needs either long code
> (over 80 chars) or some driver internal macros.
>
> There is already same issue in dmi-sysfs.c, it uses an internal macro
> DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
> there might be more for such special cases. Maybe we can add some
> comment in sysfs.h to mention this is for some special case?
>
> I can do something similar as dmi sysfs code though.
Hm, let me look at this this afternoon when I get through some stable
patches, it shouldn't be that complex to need a whole new macro...
thanks,
greg k-h
On 4 December 2017 at 09:34, Greg Kroah-Hartman
<[email protected]> wrote:
> On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
>> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
>> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
>> > > +#define __ATTR_IRUSR(_name) { \
>> > > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
>> > > + .show = _name##_show, \
>> > > +}
>> >
>> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
>> > in the ass to try to look up and remember what it is...
>> >
>> > Just use __ATTR() please, it should be fine for what you need to do,
>> > which is special-case a sysfs attribute.
>>
>> Hmm, I was hesitating to do that because it needs either long code
>> (over 80 chars) or some driver internal macros.
>>
>> There is already same issue in dmi-sysfs.c, it uses an internal macro
>> DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
>> there might be more for such special cases. Maybe we can add some
>> comment in sysfs.h to mention this is for some special case?
>>
>> I can do something similar as dmi sysfs code though.
>
> Hm, let me look at this this afternoon when I get through some stable
> patches, it shouldn't be that complex to need a whole new macro...
>
But wasn't that the whole point? That there is a macro that does what
you don't want (__ATTR_RO) and none that does what you do want?
On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
> On 4 December 2017 at 09:34, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> >> > > +#define __ATTR_IRUSR(_name) { \
> >> > > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
> >> > > + .show = _name##_show, \
> >> > > +}
> >> >
> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> >> > in the ass to try to look up and remember what it is...
> >> >
> >> > Just use __ATTR() please, it should be fine for what you need to do,
> >> > which is special-case a sysfs attribute.
> >>
> >> Hmm, I was hesitating to do that because it needs either long code
> >> (over 80 chars) or some driver internal macros.
> >>
> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
> >> DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
> >> there might be more for such special cases. Maybe we can add some
> >> comment in sysfs.h to mention this is for some special case?
> >>
> >> I can do something similar as dmi sysfs code though.
> >
> > Hm, let me look at this this afternoon when I get through some stable
> > patches, it shouldn't be that complex to need a whole new macro...
> >
>
> But wasn't that the whole point? That there is a macro that does what
> you don't want (__ATTR_RO) and none that does what you do want?
my point is that __ATTR() should work for you as-is...
On 4 December 2017 at 09:59, Greg Kroah-Hartman
<[email protected]> wrote:
> On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
>> On 4 December 2017 at 09:34, Greg Kroah-Hartman
>> <[email protected]> wrote:
>> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
>> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
>> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
>> >> > > +#define __ATTR_IRUSR(_name) { \
>> >> > > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
>> >> > > + .show = _name##_show, \
>> >> > > +}
>> >> >
>> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
>> >> > in the ass to try to look up and remember what it is...
>> >> >
>> >> > Just use __ATTR() please, it should be fine for what you need to do,
>> >> > which is special-case a sysfs attribute.
>> >>
>> >> Hmm, I was hesitating to do that because it needs either long code
>> >> (over 80 chars) or some driver internal macros.
>> >>
>> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
>> >> DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
>> >> there might be more for such special cases. Maybe we can add some
>> >> comment in sysfs.h to mention this is for some special case?
>> >>
>> >> I can do something similar as dmi sysfs code though.
>> >
>> > Hm, let me look at this this afternoon when I get through some stable
>> > patches, it shouldn't be that complex to need a whole new macro...
>> >
>>
>> But wasn't that the whole point? That there is a macro that does what
>> you don't want (__ATTR_RO) and none that does what you do want?
>
> my point is that __ATTR() should work for you as-is...
Well, not entirely.
Not sure if the runtime-map code is doing anything wrong here, but it defines
struct map_attribute {
struct attribute attr;
ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
};
and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
the .store member as well, which does not exists, and so it cannot be
used directly.
So we should either add a .store member that is always NULL, or we
should add our own
#define __ATTR_0400(_name) { \
.attr = { .name = __stringify(_name), .mode = 0400 }, \
.show = _name##_show, \
}
that does not set .store at all.
--
Ard.
On Mon, Dec 04, 2017 at 10:03:19AM +0000, Ard Biesheuvel wrote:
> On 4 December 2017 at 09:59, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Mon, Dec 04, 2017 at 09:48:37AM +0000, Ard Biesheuvel wrote:
> >> On 4 December 2017 at 09:34, Greg Kroah-Hartman
> >> <[email protected]> wrote:
> >> > On Mon, Dec 04, 2017 at 05:29:28PM +0800, Dave Young wrote:
> >> >> On 12/04/17 at 08:36am, Greg Kroah-Hartman wrote:
> >> >> > On Mon, Dec 04, 2017 at 10:02:16AM +0800, Dave Young wrote:
> >> >> > > +#define __ATTR_IRUSR(_name) { \
> >> >> > > + .attr = { .name = __stringify(_name), .mode = S_IRUSR }, \
> >> >> > > + .show = _name##_show, \
> >> >> > > +}
> >> >> >
> >> >> > Ick, no, as others, including Linus, have said, using IRUSER is a pain
> >> >> > in the ass to try to look up and remember what it is...
> >> >> >
> >> >> > Just use __ATTR() please, it should be fine for what you need to do,
> >> >> > which is special-case a sysfs attribute.
> >> >>
> >> >> Hmm, I was hesitating to do that because it needs either long code
> >> >> (over 80 chars) or some driver internal macros.
> >> >>
> >> >> There is already same issue in dmi-sysfs.c, it uses an internal macro
> >> >> DMI_SYSFS_ATTR for 0400 attr. I did not search all the kernel code,
> >> >> there might be more for such special cases. Maybe we can add some
> >> >> comment in sysfs.h to mention this is for some special case?
> >> >>
> >> >> I can do something similar as dmi sysfs code though.
> >> >
> >> > Hm, let me look at this this afternoon when I get through some stable
> >> > patches, it shouldn't be that complex to need a whole new macro...
> >> >
> >>
> >> But wasn't that the whole point? That there is a macro that does what
> >> you don't want (__ATTR_RO) and none that does what you do want?
> >
> > my point is that __ATTR() should work for you as-is...
>
> Well, not entirely.
>
> Not sure if the runtime-map code is doing anything wrong here, but it defines
>
> struct map_attribute {
> struct attribute attr;
> ssize_t (*show)(struct efi_runtime_map_entry *entry, char *buf);
> };
>
> and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> the .store member as well, which does not exists, and so it cannot be
> used directly.
>
> So we should either add a .store member that is always NULL, or we
> should add our own
You should add a .store member that is always null, as you are getting
away with a nice hack by relying on that not being present :)
> #define __ATTR_0400(_name) { \
> .attr = { .name = __stringify(_name), .mode = 0400 }, \
> .show = _name##_show, \
> }
>
> that does not set .store at all.
Creating a new macro for every different mode value you are wanting to
use seems a bit overkill, just use __ATTR() as is please. You are
already using it in other efi files today:
drivers/firmware/efi/efi.c
drivers/firmware/efi/esrt.c
thanks,
greg k-h
From: Ard Biesheuvel
> Sent: 04 December 2017 10:03
...
> and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> the .store member as well, which does not exists, and so it cannot be
> used directly.
>
> So we should either add a .store member that is always NULL, or we
> should add our own
>
> #define __ATTR_0400(_name) { \
> .attr = { .name = __stringify(_name), .mode = 0400 }, \
> .show = _name##_show, \
> }
What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
Even if the mode allowed write, writes wouldn't happen.
David
On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> From: Ard Biesheuvel
> > Sent: 04 December 2017 10:03
> ...
> > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > the .store member as well, which does not exists, and so it cannot be
> > used directly.
> >
> > So we should either add a .store member that is always NULL, or we
> > should add our own
> >
> > #define __ATTR_0400(_name) { \
> > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > .show = _name##_show, \
> > }
>
> What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> Even if the mode allowed write, writes wouldn't happen.
Ah, that might work, could you convert the other users of __ATTR() in
the efi code to use it as well?
thanks,
greg k-h
On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > From: Ard Biesheuvel
> > > Sent: 04 December 2017 10:03
> > ...
> > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > the .store member as well, which does not exists, and so it cannot be
> > > used directly.
> > >
> > > So we should either add a .store member that is always NULL, or we
> > > should add our own
> > >
> > > #define __ATTR_0400(_name) { \
> > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > .show = _name##_show, \
> > > }
> >
> > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > Even if the mode allowed write, writes wouldn't happen.
>
> Ah, that might work, could you convert the other users of __ATTR() in
> the efi code to use it as well?
$ grep __ATTR * -RI
efi.c: __ATTR(systab, 0400, systab_show, NULL);
efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
efi.c: __ATTR_RO(fw_platform_size);
esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
not necessary.
And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
I can do it but need confirm, Is this what you prefer?
Thanks
Dave
On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > From: Ard Biesheuvel
> > > > Sent: 04 December 2017 10:03
> > > ...
> > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > the .store member as well, which does not exists, and so it cannot be
> > > > used directly.
> > > >
> > > > So we should either add a .store member that is always NULL, or we
> > > > should add our own
> > > >
> > > > #define __ATTR_0400(_name) { \
> > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > .show = _name##_show, \
> > > > }
> > >
> > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > Even if the mode allowed write, writes wouldn't happen.
> >
> > Ah, that might work, could you convert the other users of __ATTR() in
> > the efi code to use it as well?
>
> $ grep __ATTR * -RI
> efi.c: __ATTR(systab, 0400, systab_show, NULL);
> efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> efi.c: __ATTR_RO(fw_platform_size);
> esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
>
> Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> not necessary.
>
> And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
>
> I can do it but need confirm, Is this what you prefer?
Yes, how about the patch below, it builds for me, haven't done anything
other than that to test it :)
Also, what's with the multi-line sysfs file systab? That's really not
allowed, can you please remove it? Also the first check for !kobj and
!buf is funny, that can never happen.
Please turn all of those different values into different sysfs files.
thanks,
greg k-h
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index f70febf680c3..c3eefa126e3b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
return str - buf;
}
-static struct kobj_attribute efi_attr_systab =
- __ATTR(systab, 0400, systab_show, NULL);
+static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
#define EFI_FIELD(var) efi.var
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index bd7ed3c1148a..7aae2483fcb9 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
};
/* Generic ESRT Entry ("ESRE") support. */
-static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
+static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
{
char *str = buf;
@@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
return str - buf;
}
-static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
- esre_fw_class_show, NULL);
+static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
#define esre_attr_decl(name, size, fmt) \
-static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
+static ssize_t name##_show(struct esre_entry *entry, char *buf) \
{ \
return sprintf(buf, fmt "\n", \
le##size##_to_cpu(entry->esre.esre1->name)); \
} \
\
-static struct esre_attribute esre_##name = __ATTR(name, 0400, \
- esre_##name##_show, NULL)
+static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
esre_attr_decl(fw_type, 32, "%u");
esre_attr_decl(fw_version, 32, "%u");
@@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
/* support for displaying ESRT fields at the top level */
#define esrt_attr_decl(name, size, fmt) \
-static ssize_t esrt_##name##_show(struct kobject *kobj, \
+static ssize_t name##_show(struct kobject *kobj, \
struct kobj_attribute *attr, char *buf)\
{ \
return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
} \
\
-static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
- esrt_##name##_show, NULL)
+static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
esrt_attr_decl(fw_resource_count, 32, "%u");
esrt_attr_decl(fw_resource_count_max, 32, "%u");
diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
index 8e64b77aeac9..f377609ff141 100644
--- a/drivers/firmware/efi/runtime-map.c
+++ b/drivers/firmware/efi/runtime-map.c
@@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
return map_attr->show(entry, buf);
}
-static struct map_attribute map_type_attr = __ATTR_RO(type);
-static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
-static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
-static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
-static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
+static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
+static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
+static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
+static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
+static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
/*
* These are default attributes that are added for every memmap entry.
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index e32dfe098e82..a886133ae15c 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -117,6 +117,11 @@ struct attribute_group {
.show = _name##_show, \
}
+#define __ATTR_RO_MODE(_name, _mode) { \
+ .attr = { .name = __stringify(_name), .mode = _mode }, \
+ .show = _name##_show, \
+}
+
#define __ATTR_WO(_name) { \
.attr = { .name = __stringify(_name), .mode = S_IWUSR }, \
.store = _name##_store, \
On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > From: Ard Biesheuvel
> > > > > Sent: 04 December 2017 10:03
> > > > ...
> > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > used directly.
> > > > >
> > > > > So we should either add a .store member that is always NULL, or we
> > > > > should add our own
> > > > >
> > > > > #define __ATTR_0400(_name) { \
> > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > .show = _name##_show, \
> > > > > }
> > > >
> > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > Even if the mode allowed write, writes wouldn't happen.
> > >
> > > Ah, that might work, could you convert the other users of __ATTR() in
> > > the efi code to use it as well?
> >
> > $ grep __ATTR * -RI
> > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > efi.c: __ATTR_RO(fw_platform_size);
> > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> >
> > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> > not necessary.
> >
> > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> >
> > I can do it but need confirm, Is this what you prefer?
>
> Yes, how about the patch below, it builds for me, haven't done anything
> other than that to test it :)
Thanks! Let me do a kexec test and a boot test for esrt.
>
> Also, what's with the multi-line sysfs file systab? That's really not
> allowed, can you please remove it? Also the first check for !kobj and
> !buf is funny, that can never happen.
I thought to do that, but later worried about it will break things:
http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
I also thought to add code comment to avoid future expanding of this
file. Maybe we can do this now.
>
> Please turn all of those different values into different sysfs files.
>
> thanks,
>
> greg k-h
>
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index f70febf680c3..c3eefa126e3b 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
> return str - buf;
> }
>
> -static struct kobj_attribute efi_attr_systab =
> - __ATTR(systab, 0400, systab_show, NULL);
> +static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
>
> #define EFI_FIELD(var) efi.var
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index bd7ed3c1148a..7aae2483fcb9 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
> };
>
> /* Generic ESRT Entry ("ESRE") support. */
> -static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> +static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
> {
> char *str = buf;
>
> @@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> return str - buf;
> }
>
> -static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> - esre_fw_class_show, NULL);
> +static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
>
> #define esre_attr_decl(name, size, fmt) \
> -static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
> +static ssize_t name##_show(struct esre_entry *entry, char *buf) \
> { \
> return sprintf(buf, fmt "\n", \
> le##size##_to_cpu(entry->esre.esre1->name)); \
> } \
> \
> -static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> - esre_##name##_show, NULL)
> +static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
>
> esre_attr_decl(fw_type, 32, "%u");
> esre_attr_decl(fw_version, 32, "%u");
> @@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
>
> /* support for displaying ESRT fields at the top level */
> #define esrt_attr_decl(name, size, fmt) \
> -static ssize_t esrt_##name##_show(struct kobject *kobj, \
> +static ssize_t name##_show(struct kobject *kobj, \
> struct kobj_attribute *attr, char *buf)\
> { \
> return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
> } \
> \
> -static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> - esrt_##name##_show, NULL)
> +static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
>
> esrt_attr_decl(fw_resource_count, 32, "%u");
> esrt_attr_decl(fw_resource_count_max, 32, "%u");
> diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> index 8e64b77aeac9..f377609ff141 100644
> --- a/drivers/firmware/efi/runtime-map.c
> +++ b/drivers/firmware/efi/runtime-map.c
> @@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
> return map_attr->show(entry, buf);
> }
>
> -static struct map_attribute map_type_attr = __ATTR_RO(type);
> -static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> -static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> -static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> -static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> +static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
> +static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
> +static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
> +static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
> +static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
>
> /*
> * These are default attributes that are added for every memmap entry.
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index e32dfe098e82..a886133ae15c 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -117,6 +117,11 @@ struct attribute_group {
> .show = _name##_show, \
> }
>
> +#define __ATTR_RO_MODE(_name, _mode) { \
> + .attr = { .name = __stringify(_name), .mode = _mode }, \
> + .show = _name##_show, \
> +}
> +
> #define __ATTR_WO(_name) { \
> .attr = { .name = __stringify(_name), .mode = S_IWUSR }, \
> .store = _name##_store, \
Thanks
Dave
On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > From: Ard Biesheuvel
> > > > > > Sent: 04 December 2017 10:03
> > > > > ...
> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > used directly.
> > > > > >
> > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > should add our own
> > > > > >
> > > > > > #define __ATTR_0400(_name) { \
> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > .show = _name##_show, \
> > > > > > }
> > > > >
> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > Even if the mode allowed write, writes wouldn't happen.
> > > >
> > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > the efi code to use it as well?
> > >
> > > $ grep __ATTR * -RI
> > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > efi.c: __ATTR_RO(fw_platform_size);
> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > >
> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> > > not necessary.
> > >
> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > >
> > > I can do it but need confirm, Is this what you prefer?
> >
> > Yes, how about the patch below, it builds for me, haven't done anything
> > other than that to test it :)
>
> Thanks! Let me do a kexec test and a boot test for esrt.
>
> >
> > Also, what's with the multi-line sysfs file systab? That's really not
> > allowed, can you please remove it? Also the first check for !kobj and
> > !buf is funny, that can never happen.
>
> I thought to do that, but later worried about it will break things:
> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
Heh, I guess I complained about this in the past :)
So what userspace tool uses it?
Are these values all exported through sysfs already? If not, do that
first.
> I also thought to add code comment to avoid future expanding of this
> file. Maybe we can do this now.
Please do, but it should be a separate patch.
thanks,
greg k-h
On 12/05/17 at 04:45pm, Dave Young wrote:
> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > From: Ard Biesheuvel
> > > > > > Sent: 04 December 2017 10:03
> > > > > ...
> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > used directly.
> > > > > >
> > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > should add our own
> > > > > >
> > > > > > #define __ATTR_0400(_name) { \
> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > .show = _name##_show, \
> > > > > > }
> > > > >
> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > Even if the mode allowed write, writes wouldn't happen.
> > > >
> > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > the efi code to use it as well?
> > >
> > > $ grep __ATTR * -RI
> > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > efi.c: __ATTR_RO(fw_platform_size);
> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > >
> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> > > not necessary.
> > >
> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > >
> > > I can do it but need confirm, Is this what you prefer?
> >
> > Yes, how about the patch below, it builds for me, haven't done anything
> > other than that to test it :)
>
> Thanks! Let me do a kexec test and a boot test for esrt.
It works for me. esrt part only did a boot test and cat/ls.
>
> >
> > Also, what's with the multi-line sysfs file systab? That's really not
> > allowed, can you please remove it? Also the first check for !kobj and
> > !buf is funny, that can never happen.
>
> I thought to do that, but later worried about it will break things:
> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
>
> I also thought to add code comment to avoid future expanding of this
> file. Maybe we can do this now.
>
> >
> > Please turn all of those different values into different sysfs files.
> >
> > thanks,
> >
> > greg k-h
> >
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index f70febf680c3..c3eefa126e3b 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -143,8 +143,7 @@ static ssize_t systab_show(struct kobject *kobj,
> > return str - buf;
> > }
> >
> > -static struct kobj_attribute efi_attr_systab =
> > - __ATTR(systab, 0400, systab_show, NULL);
> > +static struct kobj_attribute efi_attr_systab = __ATTR_RO_MODE(systab, 0400);
> >
> > #define EFI_FIELD(var) efi.var
> >
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index bd7ed3c1148a..7aae2483fcb9 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -106,7 +106,7 @@ static const struct sysfs_ops esre_attr_ops = {
> > };
> >
> > /* Generic ESRT Entry ("ESRE") support. */
> > -static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> > +static ssize_t fw_class_show(struct esre_entry *entry, char *buf)
> > {
> > char *str = buf;
> >
> > @@ -117,18 +117,16 @@ static ssize_t esre_fw_class_show(struct esre_entry *entry, char *buf)
> > return str - buf;
> > }
> >
> > -static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > - esre_fw_class_show, NULL);
> > +static struct esre_attribute esre_fw_class = __ATTR_RO_MODE(fw_class, 0400);
> >
> > #define esre_attr_decl(name, size, fmt) \
> > -static ssize_t esre_##name##_show(struct esre_entry *entry, char *buf) \
> > +static ssize_t name##_show(struct esre_entry *entry, char *buf) \
> > { \
> > return sprintf(buf, fmt "\n", \
> > le##size##_to_cpu(entry->esre.esre1->name)); \
> > } \
> > \
> > -static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > - esre_##name##_show, NULL)
> > +static struct esre_attribute esre_##name = __ATTR_RO_MODE(name, 0400)
> >
> > esre_attr_decl(fw_type, 32, "%u");
> > esre_attr_decl(fw_version, 32, "%u");
> > @@ -193,14 +191,13 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
> >
> > /* support for displaying ESRT fields at the top level */
> > #define esrt_attr_decl(name, size, fmt) \
> > -static ssize_t esrt_##name##_show(struct kobject *kobj, \
> > +static ssize_t name##_show(struct kobject *kobj, \
> > struct kobj_attribute *attr, char *buf)\
> > { \
> > return sprintf(buf, fmt "\n", le##size##_to_cpu(esrt->name)); \
> > } \
> > \
> > -static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > - esrt_##name##_show, NULL)
> > +static struct kobj_attribute esrt_##name = __ATTR_RO_MODE(name, 0400)
> >
> > esrt_attr_decl(fw_resource_count, 32, "%u");
> > esrt_attr_decl(fw_resource_count_max, 32, "%u");
> > diff --git a/drivers/firmware/efi/runtime-map.c b/drivers/firmware/efi/runtime-map.c
> > index 8e64b77aeac9..f377609ff141 100644
> > --- a/drivers/firmware/efi/runtime-map.c
> > +++ b/drivers/firmware/efi/runtime-map.c
> > @@ -63,11 +63,11 @@ static ssize_t map_attr_show(struct kobject *kobj, struct attribute *attr,
> > return map_attr->show(entry, buf);
> > }
> >
> > -static struct map_attribute map_type_attr = __ATTR_RO(type);
> > -static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > -static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > -static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > -static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > +static struct map_attribute map_type_attr = __ATTR_RO_MODE(type, 0400);
> > +static struct map_attribute map_phys_addr_attr = __ATTR_RO_MODE(phys_addr, 0400);
> > +static struct map_attribute map_virt_addr_attr = __ATTR_RO_MODE(virt_addr, 0400);
> > +static struct map_attribute map_num_pages_attr = __ATTR_RO_MODE(num_pages, 0400);
> > +static struct map_attribute map_attribute_attr = __ATTR_RO_MODE(attribute, 0400);
> >
> > /*
> > * These are default attributes that are added for every memmap entry.
> > diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> > index e32dfe098e82..a886133ae15c 100644
> > --- a/include/linux/sysfs.h
> > +++ b/include/linux/sysfs.h
> > @@ -117,6 +117,11 @@ struct attribute_group {
> > .show = _name##_show, \
> > }
> >
> > +#define __ATTR_RO_MODE(_name, _mode) { \
> > + .attr = { .name = __stringify(_name), .mode = _mode }, \
> > + .show = _name##_show, \
> > +}
> > +
> > #define __ATTR_WO(_name) { \
> > .attr = { .name = __stringify(_name), .mode = S_IWUSR }, \
> > .store = _name##_store, \
>
> Thanks
> Dave
On 5 December 2017 at 08:52, Greg Kroah-Hartman
<[email protected]> wrote:
> On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
>> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
>> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
>> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
>> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
>> > > > > From: Ard Biesheuvel
>> > > > > > Sent: 04 December 2017 10:03
>> > > > > ...
>> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
>> > > > > > the .store member as well, which does not exists, and so it cannot be
>> > > > > > used directly.
>> > > > > >
>> > > > > > So we should either add a .store member that is always NULL, or we
>> > > > > > should add our own
>> > > > > >
>> > > > > > #define __ATTR_0400(_name) { \
>> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
>> > > > > > .show = _name##_show, \
>> > > > > > }
>> > > > >
>> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
>> > > > > Even if the mode allowed write, writes wouldn't happen.
>> > > >
>> > > > Ah, that might work, could you convert the other users of __ATTR() in
>> > > > the efi code to use it as well?
>> > >
>> > > $ grep __ATTR * -RI
>> > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
>> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
>> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
>> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>> > > efi.c: __ATTR_RO(fw_platform_size);
>> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
>> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
>> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
>> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
>> > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
>> > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
>> > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
>> > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
>> > >
>> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
>> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
>> > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
>> > > not necessary.
>> > >
>> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
>> > >
>> > > I can do it but need confirm, Is this what you prefer?
>> >
>> > Yes, how about the patch below, it builds for me, haven't done anything
>> > other than that to test it :)
>>
>> Thanks! Let me do a kexec test and a boot test for esrt.
>>
>> >
>> > Also, what's with the multi-line sysfs file systab? That's really not
>> > allowed, can you please remove it? Also the first check for !kobj and
>> > !buf is funny, that can never happen.
>>
>> I thought to do that, but later worried about it will break things:
>> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
>
> Heh, I guess I complained about this in the past :)
>
> So what userspace tool uses it?
>
On x86, it is mostly tools that read DMI tables via /dev/mem, and use
/sys/firmware/efi/systab to locate them. dmidecode, lscpu, etc
That does mean we could investigate which entries are actually used,
and at least start removing the ones we don't need.
> Are these values all exported through sysfs already? If not, do that
> first.
>
>> I also thought to add code comment to avoid future expanding of this
>> file. Maybe we can do this now.
>
> Please do, but it should be a separate patch.
>
> thanks,
>
> greg k-h
On 12/05/17 at 09:52am, Greg Kroah-Hartman wrote:
> On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> > On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > > From: Ard Biesheuvel
> > > > > > > Sent: 04 December 2017 10:03
> > > > > > ...
> > > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > > used directly.
> > > > > > >
> > > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > > should add our own
> > > > > > >
> > > > > > > #define __ATTR_0400(_name) { \
> > > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > > .show = _name##_show, \
> > > > > > > }
> > > > > >
> > > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > >
> > > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > > the efi code to use it as well?
> > > >
> > > > $ grep __ATTR * -RI
> > > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> > > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > > efi.c: __ATTR_RO(fw_platform_size);
> > > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > > >
> > > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> > > > not necessary.
> > > >
> > > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > >
> > > > I can do it but need confirm, Is this what you prefer?
> > >
> > > Yes, how about the patch below, it builds for me, haven't done anything
> > > other than that to test it :)
> >
> > Thanks! Let me do a kexec test and a boot test for esrt.
> >
> > >
> > > Also, what's with the multi-line sysfs file systab? That's really not
> > > allowed, can you please remove it? Also the first check for !kobj and
> > > !buf is funny, that can never happen.
> >
> > I thought to do that, but later worried about it will break things:
> > http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
>
> Heh, I guess I complained about this in the past :)
>
> So what userspace tool uses it?
>
> Are these values all exported through sysfs already? If not, do that
> first.
kexec-tools uses it, ie. SMBIOS/ACPI/ACPI20 which was exported before.
Later we added files out of systab though.
Since there are also other fields like MPS/BOOTINFO/UGA etc I'm not sure
if something else has been using it as well..
>
> > I also thought to add code comment to avoid future expanding of this
> > file. Maybe we can do this now.
>
> Please do, but it should be a separate patch.
Ok, will send it and cc you.
>
> thanks,
>
> greg k-h
Thanks
Dave
On Tue, Dec 05, 2017 at 05:24:10PM +0800, Dave Young wrote:
> On 12/05/17 at 04:45pm, Dave Young wrote:
> > On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> > > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> > > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> > > > > > From: Ard Biesheuvel
> > > > > > > Sent: 04 December 2017 10:03
> > > > > > ...
> > > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> > > > > > > the .store member as well, which does not exists, and so it cannot be
> > > > > > > used directly.
> > > > > > >
> > > > > > > So we should either add a .store member that is always NULL, or we
> > > > > > > should add our own
> > > > > > >
> > > > > > > #define __ATTR_0400(_name) { \
> > > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> > > > > > > .show = _name##_show, \
> > > > > > > }
> > > > > >
> > > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> > > > > > Even if the mode allowed write, writes wouldn't happen.
> > > > >
> > > > > Ah, that might work, could you convert the other users of __ATTR() in
> > > > > the efi code to use it as well?
> > > >
> > > > $ grep __ATTR * -RI
> > > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> > > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > > > efi.c: __ATTR_RO(fw_platform_size);
> > > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> > > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> > > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> > > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> > > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> > > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> > > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> > > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> > > >
> > > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> > > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> > > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> > > > not necessary.
> > > >
> > > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> > > >
> > > > I can do it but need confirm, Is this what you prefer?
> > >
> > > Yes, how about the patch below, it builds for me, haven't done anything
> > > other than that to test it :)
> >
> > Thanks! Let me do a kexec test and a boot test for esrt.
>
> It works for me. esrt part only did a boot test and cat/ls.
Great, thanks for testing, I've sent it off now as a "real" patch.
greg k-h
On Tue, Dec 05, 2017 at 09:25:07AM +0000, Ard Biesheuvel wrote:
> On 5 December 2017 at 08:52, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Tue, Dec 05, 2017 at 04:45:37PM +0800, Dave Young wrote:
> >> On 12/05/17 at 09:09am, Greg Kroah-Hartman wrote:
> >> > On Tue, Dec 05, 2017 at 01:14:34PM +0800, Dave Young wrote:
> >> > > On 12/04/17 at 03:00pm, Greg Kroah-Hartman wrote:
> >> > > > On Mon, Dec 04, 2017 at 12:51:13PM +0000, David Laight wrote:
> >> > > > > From: Ard Biesheuvel
> >> > > > > > Sent: 04 December 2017 10:03
> >> > > > > ...
> >> > > > > > and uses __ATTR_RO() to emit initializers for it. __ATTR() initializes
> >> > > > > > the .store member as well, which does not exists, and so it cannot be
> >> > > > > > used directly.
> >> > > > > >
> >> > > > > > So we should either add a .store member that is always NULL, or we
> >> > > > > > should add our own
> >> > > > > >
> >> > > > > > #define __ATTR_0400(_name) { \
> >> > > > > > .attr = { .name = __stringify(_name), .mode = 0400 }, \
> >> > > > > > .show = _name##_show, \
> >> > > > > > }
> >> > > > >
> >> > > > > What about an __ATTR_RO_MODE(name, mode) that doesn't set the .store member.
> >> > > > > Even if the mode allowed write, writes wouldn't happen.
> >> > > >
> >> > > > Ah, that might work, could you convert the other users of __ATTR() in
> >> > > > the efi code to use it as well?
> >> > >
> >> > > $ grep __ATTR * -RI
> >> > > efi.c: __ATTR(systab, 0400, systab_show, NULL);
> >> > > efi.c:static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> >> > > efi.c:static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> >> > > efi.c:static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> >> > > efi.c: __ATTR_RO(fw_platform_size);
> >> > > esrt.c:static struct esre_attribute esre_fw_class = __ATTR(fw_class, 0400,
> >> > > esrt.c:static struct esre_attribute esre_##name = __ATTR(name, 0400, \
> >> > > esrt.c:static struct kobj_attribute esrt_##name = __ATTR(name, 0400, \
> >> > > runtime-map.c:static struct map_attribute map_type_attr = __ATTR_RO(type);
> >> > > runtime-map.c:static struct map_attribute map_phys_addr_attr = __ATTR_RO(phys_addr);
> >> > > runtime-map.c:static struct map_attribute map_virt_addr_attr = __ATTR_RO(virt_addr);
> >> > > runtime-map.c:static struct map_attribute map_num_pages_attr = __ATTR_RO(num_pages);
> >> > > runtime-map.c:static struct map_attribute map_attribute_attr = __ATTR_RO(attribute);
> >> > >
> >> > > Above is all the __ATTR users for drivers/firmware/efi/*, it makes sense
> >> > > to update all __ATTR_RO to __ATTR_RO_MODE, so efi.c, runtime-map.c, and
> >> > > drivers/firmware/dmi-sysfs.c can be updated. But esrt.c __ATTR seems
> >> > > not necessary.
> >> > >
> >> > > And if so __ATTR_RO_MODE(name, mode) still needs go to sysfs.h.
> >> > >
> >> > > I can do it but need confirm, Is this what you prefer?
> >> >
> >> > Yes, how about the patch below, it builds for me, haven't done anything
> >> > other than that to test it :)
> >>
> >> Thanks! Let me do a kexec test and a boot test for esrt.
> >>
> >> >
> >> > Also, what's with the multi-line sysfs file systab? That's really not
> >> > allowed, can you please remove it? Also the first check for !kobj and
> >> > !buf is funny, that can never happen.
> >>
> >> I thought to do that, but later worried about it will break things:
> >> http://lists.infradead.org/pipermail/kexec/2013-December/010759.html
> >
> > Heh, I guess I complained about this in the past :)
> >
> > So what userspace tool uses it?
> >
>
> On x86, it is mostly tools that read DMI tables via /dev/mem, and use
> /sys/firmware/efi/systab to locate them. dmidecode, lscpu, etc
>
> That does mean we could investigate which entries are actually used,
> and at least start removing the ones we don't need.
That would be good to do. Also, the data there is already covered by
other sysfs files, right? If not, you should add that as individual
files first.
thanks,
greg k-h