[PATCH] fix system_state checking in early_ioremap

[Dave Young]

Since below commit earlyprintk=efi,keep does not work any more with a warning
in mm/early_ioremap.c: WARN_ON(system_state >= SYSTEM_RUNNING):
commit 69a78ff226fe ("init: Introduce SYSTEM_SCHEDULING state")

Reason is the the original assumption is SYSTEM_BOOTING equal to
system_state < SYSTEM_RUNNING. But with commit 69a78ff226fe it is not true
any more. Change the WARN_ON to check system_state >= SYSTEM_RUNNING instead.

Signed-off-by: Dave Young 
---
 mm/early_ioremap.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-x86.orig/mm/early_ioremap.c
+++ linux-x86/mm/early_ioremap.c
@@ -111,7 +111,7 @@ __early_ioremap(resource_size_t phys_add
enum fixed_addresses idx;
int i, slot;
 
-   WARN_ON(system_state != SYSTEM_BOOTING);
+   WARN_ON(system_state >= SYSTEM_RUNNING);
 
slot = -1;
for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kees Cook]

On Fri, Dec 1, 2017 at 7:34 AM, Greg Kroah-Hartman
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg Kroah-Hartman]

On Fri, Dec 01, 2017 at 09:54:43AM +, Ard Biesheuvel wrote:
> On 1 December 2017 at 09:48, Greg Kroah-Hartman
>  wrote:
> > On Thu, Nov 30, 2017 at 05:18:42PM +, Ard Biesheuvel wrote:
> >> On 30 November 2017 at 17:10, Greg Kroah-Hartman
> >>  wrote:
> >> > On Thu, Nov 30, 2017 at 04:32:35PM +, Greg Kroah-Hartman wrote:
> >> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> >> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> >> >> >  wrote:
> >> >> > >
> >> >> > > Not because %pK itself changed, but because the semantics of %p did.
> >> >> > > The baseline moved, and the "safe" version did not.
> >> >> >
> >> >> > Btw, that baseline for me is now that I can do
> >> >> >
> >> >> >   ./scripts/leaking_addresses.pl | wc -l
> >> >> >   18
> >> >> >
> >> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> >> >> > the uevent keys).
> >> >> >
> >> >> > The remaining 12 are from the EFI runtime map files
> >> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> >> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> >> >> > that by default.
> >> >> >
> >> >> > I think the sysfs code makes it insanely too easy to make things
> >> >> > world-readable. You try to be careful, and mark things read-only etc,
> >> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> >> >> >
> >> >> > There seems to be no convenient model for kobjects having better
> >> >> > permissions. Greg?
> >> >>
> >> >> They can just use __ATTR() which lets you set the exact mode settings
> >> >> that are wanted.
> >> >>
> >> >> Something like the patch below, which breaks the build as the
> >> >> map_attributes are "odd", but you get the idea.  The EFI developers can
> >> >> fix this up properly :)
> >> >>
> >> >> Note, this only accounts for 5 attributes, what is the whole list?
> >> >
> >> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> >> >
> >> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffeea6ea000
> >> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffeee88b000
> >> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffefea0
> >> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffed9c0
> >> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffefee0
> >> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffedba4e000
> >> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffeee2de000
> >> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffeeea0
> >> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffefec0
> >> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffed9c6
> >> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffeff00
> >> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffedb9c9000
> >> >
> >> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] arm64: Add software workaround for Falkor erratum 1041

[Will Deacon]

On Mon, Nov 27, 2017 at 05:18:00PM -0600, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter 4K and next 4K.
> 
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
> 
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>to the errant memory access attempting to access a region of memory
>that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>memory. This behavior may only occur if the instruction cache is
>disabled prior to or coincident with translation being changed from
>enabled to disabled.
> 
> The conditions leading to this erratum will not occur when either of the
> following occur:
>  1) A higher exception level disables translation of a lower exception level
>(e.g. EL2 changing SCTLR_EL1[M] from a value of 1 to 0).
>  2) An exception level disabling its stage-1 translation if its stage-2
> translation is enabled (e.g. EL1 changing SCTLR_EL1[M] from a value of 1
> to 0 when HCR_EL2[VM] has a value of 1).
> 
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> Signed-off-by: Shanker Donthineni 
> ---
> Changes since v3:
>   Rebased to kernel v4.15-rc1.
> Changes since v2:
>   Repost the corrected patches.
> Changes since v1:
>   Apply the workaround where it's required.
> 
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig | 12 +++-
>  arch/arm64/include/asm/assembler.h | 19 +++
>  arch/arm64/include/asm/cpucaps.h   |  3 ++-
>  arch/arm64/kernel/cpu-reset.S  |  1 +
>  arch/arm64/kernel/cpu_errata.c | 16 
>  arch/arm64/kernel/efi-entry.S  |  2 ++
>  arch/arm64/kernel/head.S   |  1 +
>  arch/arm64/kernel/relocate_kernel.S|  1 +
>  arch/arm64/kvm/hyp-init.S  |  1 +

This is an awful lot of code just to add an ISB instruction prior to
disabling the MMU. Why do you need to go through the alternatives framework
for this? Just do it with an #ifdef; this isn't a fastpath.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ard Biesheuvel]

On 1 December 2017 at 09:48, Greg Kroah-Hartman
 wrote:
> On Thu, Nov 30, 2017 at 05:18:42PM +, Ard Biesheuvel wrote:
>> On 30 November 2017 at 17:10, Greg Kroah-Hartman
>>  wrote:
>> > On Thu, Nov 30, 2017 at 04:32:35PM +, Greg Kroah-Hartman wrote:
>> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
>> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
>> >> >  wrote:
>> >> > >
>> >> > > Not because %pK itself changed, but because the semantics of %p did.
>> >> > > The baseline moved, and the "safe" version did not.
>> >> >
>> >> > Btw, that baseline for me is now that I can do
>> >> >
>> >> >   ./scripts/leaking_addresses.pl | wc -l
>> >> >   18
>> >> >
>> >> > and of those 18 hits, six are false positives (looks like bitmaps in
>> >> > the uevent keys).
>> >> >
>> >> > The remaining 12 are from the EFI runtime map files
>> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
>> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
>> >> > that by default.
>> >> >
>> >> > I think the sysfs code makes it insanely too easy to make things
>> >> > world-readable. You try to be careful, and mark things read-only etc,
>> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
>> >> >
>> >> > There seems to be no convenient model for kobjects having better
>> >> > permissions. Greg?
>> >>
>> >> They can just use __ATTR() which lets you set the exact mode settings
>> >> that are wanted.
>> >>
>> >> Something like the patch below, which breaks the build as the
>> >> map_attributes are "odd", but you get the idea.  The EFI developers can
>> >> fix this up properly :)
>> >>
>> >> Note, this only accounts for 5 attributes, what is the whole list?
>> >
>> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
>> >
>> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffeea6ea000
>> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffeee88b000
>> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffefea0
>> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffed9c0
>> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffefee0
>> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffedba4e000
>> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffeee2de000
>> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffeeea0
>> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffefec0
>> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffed9c6
>> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffeff00
>> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffedb9c9000
>> >
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg Kroah-Hartman]

On Thu, Nov 30, 2017 at 05:18:42PM +, Ard Biesheuvel wrote:
> On 30 November 2017 at 17:10, Greg Kroah-Hartman
>  wrote:
> > On Thu, Nov 30, 2017 at 04:32:35PM +, Greg Kroah-Hartman wrote:
> >> On Wed, Nov 29, 2017 at 01:36:25PM -0800, Linus Torvalds wrote:
> >> > On Wed, Nov 29, 2017 at 1:14 PM, Linus Torvalds
> >> >  wrote:
> >> > >
> >> > > Not because %pK itself changed, but because the semantics of %p did.
> >> > > The baseline moved, and the "safe" version did not.
> >> >
> >> > Btw, that baseline for me is now that I can do
> >> >
> >> >   ./scripts/leaking_addresses.pl | wc -l
> >> >   18
> >> >
> >> > and of those 18 hits, six are false positives (looks like bitmaps in
> >> > the uevent keys).
> >> >
> >> > The remaining 12 are from the EFI runtime map files
> >> > (/sys/firmware/efi/runtime-map/*). They should presumably not be
> >> > world-readable, but sadly the kset_create_and_add() helper seems to do
> >> > that by default.
> >> >
> >> > I think the sysfs code makes it insanely too easy to make things
> >> > world-readable. You try to be careful, and mark things read-only etc,
> >> > but __ATTR_RO() jkust means S_IRUGO, which means world-readable.
> >> >
> >> > There seems to be no convenient model for kobjects having better
> >> > permissions. Greg?
> >>
> >> They can just use __ATTR() which lets you set the exact mode settings
> >> that are wanted.
> >>
> >> Something like the patch below, which breaks the build as the
> >> map_attributes are "odd", but you get the idea.  The EFI developers can
> >> fix this up properly :)
> >>
> >> Note, this only accounts for 5 attributes, what is the whole list?
> >
> > Ah, it's the virt_addr file 12 times, I just ran it on my laptop:
> >
> > /sys/firmware/efi/runtime-map/7/virt_addr: 0xfffeea6ea000
> > /sys/firmware/efi/runtime-map/5/virt_addr: 0xfffeee88b000
> > /sys/firmware/efi/runtime-map/3/virt_addr: 0xfffefea0
> > /sys/firmware/efi/runtime-map/11/virt_addr: 0xfffed9c0
> > /sys/firmware/efi/runtime-map/1/virt_addr: 0xfffefee0
> > /sys/firmware/efi/runtime-map/8/virt_addr: 0xfffedba4e000
> > /sys/firmware/efi/runtime-map/6/virt_addr: 0xfffeee2de000
> > /sys/firmware/efi/runtime-map/4/virt_addr: 0xfffeeea0
> > /sys/firmware/efi/runtime-map/2/virt_addr: 0xfffefec0
> > /sys/firmware/efi/runtime-map/10/virt_addr: 0xfffed9c6
> > /sys/firmware/efi/runtime-map/0/virt_addr: 0xfffeff00
> > /sys/firmware/efi/runtime-map/9/virt_addr: 0xfffedb9c9000
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg Kroah-Hartman]

On Thu, Nov 30, 2017 at 06:17:47PM -0500, Linus Torvalds wrote:
> On Thu, Nov 30, 2017 at 12:10 PM, Greg Kroah-Hartman
>  wrote:
> >
> > So changing it to use __ATTR() should fix this remaning leakage up.
> > That is if we even really need to export these values at all.  What does
> > userspace do with them?  Shouldn't they just be in debugfs instead?
> 
> So what I find distasteful here is how sysfs has these "helper" macros
> that are clearly designed to over-share.

That is by design :)

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

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

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

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html