[PATCH] arm64: fix kernel panic on serror caused by user process

[PATCH] arm64: fix kernel panic on serror caused by user process

[Hari Vyas]

changes in v1:
	Based on processor mode, action is taken i.e. for user mode, only
	user process is killed otherwise as usual kernel will panic and
	system halts.

Hari Vyas (1):
  arm64: fix kernel panic on serror exception caused by user process

 arch/arm64/kernel/traps.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
1.9.1

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Hari Vyas]

On executing simple user level "devmem 0x0" command, kernel panics.
As 0x0 address is mostly not matched to any valid memory so exception
is expected and raised which results in unconditional kernel panic
by serror handler.

This is happening after newly introduced serror handling framework
change which panics kernel on any any serror without checking
processor mode.`

Kernel panic is fixed by checking processor mode in serror handler.
On kernel mode, normal kernel panic action is taken and system halts.
On user mode, only user process is killed and further panic action is
not initiated.

Signed-off-by: Hari Vyas <hari.vyas@broadcom.com>
---
 arch/arm64/kernel/traps.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index d399d45..c7cbad7 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -729,6 +729,13 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
 
 asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
 {
+	if (user_mode(regs)) {
+		pr_crit("UserMode SError Exception on CPU%d, code 0x%08x %s\n",
+			smp_processor_id(), esr, esr_get_class_string(esr));
+		die("Oops - user mode ", regs, 0);
+		return;
+        }
+
 	nmi_enter();
 
 	/* non-RAS errors are not containable */
-- 
1.9.1

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Mark Rutland]

On Tue, Jul 17, 2018 at 03:01:21PM +0530, Hari Vyas wrote:
> On executing simple user level "devmem 0x0" command, kernel panics.
> As 0x0 address is mostly not matched to any valid memory so exception
> is expected and raised which results in unconditional kernel panic
> by serror handler.

Having access to /dev/mem means that userspace can bring down the system
in any number of ways.

Why did userspace do this, and why do you think this shouldn't be fatal?

> This is happening after newly introduced serror handling framework
> change which panics kernel on any any serror without checking
> processor mode.`

This is the expected behaviour. The processor mode is not relevant,
because Serror is asynchronous -- so we cannot attribute it to userspace
instructions.

> Kernel panic is fixed by checking processor mode in serror handler.
> On kernel mode, normal kernel panic action is taken and system halts.
> On user mode, only user process is killed and further panic action is
> not initiated.

This is not safe.

For example, an Serror could result from the kernel page tables being
programmed to point at device memory. A TLB walk for TTBR1 made while
userspace was executing could result in an SError, and killing userspace
alone is insufficient to contain the error.

Thanks,
Mark.

> 
> Signed-off-by: Hari Vyas <hari.vyas@broadcom.com>
> ---
>  arch/arm64/kernel/traps.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index d399d45..c7cbad7 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -729,6 +729,13 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
>  
>  asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
>  {
> +	if (user_mode(regs)) {
> +		pr_crit("UserMode SError Exception on CPU%d, code 0x%08x %s\n",
> +			smp_processor_id(), esr, esr_get_class_string(esr));
> +		die("Oops - user mode ", regs, 0);
> +		return;
> +        }
> +
>  	nmi_enter();
>  
>  	/* non-RAS errors are not containable */
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Hari Vyas]

On Tue, Jul 17, 2018 at 3:36 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 17, 2018 at 03:01:21PM +0530, Hari Vyas wrote:
>> On executing simple user level "devmem 0x0" command, kernel panics.
>> As 0x0 address is mostly not matched to any valid memory so exception
>> is expected and raised which results in unconditional kernel panic
>> by serror handler.
>
> Having access to /dev/mem means that userspace can bring down the system
> in any number of ways.
>
> Why did userspace do this, and why do you think this shouldn't be fatal?
>
I understand,  here /dev/mem is accessible but access is issued by
user application devmem (which user can even do by mistake or
deliberately) and
don't think system/kernel should be  penalized for such type of minor mistake.
>> This is happening after newly introduced serror handling framework
>> change which panics kernel on any any serror without checking
>> processor mode.`
>
> This is the expected behaviour. The processor mode is not relevant,
> because Serror is asynchronous -- so we cannot attribute it to userspace
> instructions.
>
We recently moved to 4.17 version but if my understanding is correct
till  Kernel 4.14, same access was resulting in bad mode and due to
user process initiation,
kernel was throwing exception and devmem was killed but  not
resulting in kernel panic.

>> Kernel panic is fixed by checking processor mode in serror handler.
>> On kernel mode, normal kernel panic action is taken and system halts.
>> On user mode, only user process is killed and further panic action is
>> not initiated.
>
> This is not safe.
>
> For example, an Serror could result from the kernel page tables being
> programmed to point at device memory. A TLB walk for TTBR1 made while
> userspace was executing could result in an SError, and killing userspace
> alone is insufficient to contain the error.
>
Yes. Serror exception just gives indication and needs to be further
analyzed but here case is direct and known and can be handled.

> Thanks,
> Mark.
>
>>
>> Signed-off-by: Hari Vyas <hari.vyas@broadcom.com>
>> ---
>>  arch/arm64/kernel/traps.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index d399d45..c7cbad7 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -729,6 +729,13 @@ bool arm64_is_fatal_ras_serror(struct pt_regs *regs, unsigned int esr)
>>
>>  asmlinkage void do_serror(struct pt_regs *regs, unsigned int esr)
>>  {
>> +     if (user_mode(regs)) {
>> +             pr_crit("UserMode SError Exception on CPU%d, code 0x%08x %s\n",
>> +                     smp_processor_id(), esr, esr_get_class_string(esr));
>> +             die("Oops - user mode ", regs, 0);
>> +             return;
>> +        }
>> +
>>       nmi_enter();
>>
>>       /* non-RAS errors are not containable */
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Mark Rutland]

On Tue, Jul 17, 2018 at 05:02:04PM +0530, Hari Vyas wrote:
> On Tue, Jul 17, 2018 at 3:36 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Jul 17, 2018 at 03:01:21PM +0530, Hari Vyas wrote:
> >> On executing simple user level "devmem 0x0" command, kernel panics.
> >> As 0x0 address is mostly not matched to any valid memory so exception
> >> is expected and raised which results in unconditional kernel panic
> >> by serror handler.
> >
> > Having access to /dev/mem means that userspace can bring down the system
> > in any number of ways.
> >
> > Why did userspace do this, and why do you think this shouldn't be fatal?
> >
> I understand,  here /dev/mem is accessible but access is issued by
> user application devmem (which user can even do by mistake or
> deliberately) and
> don't think system/kernel should be  penalized for such type of minor mistake.

Sorry, but this is simply a risk of exposing /dev/mem to userspace.

The user could also use devmem to poke devices in ways which could
permanently damage them. If you cannot trust the user to not do such
things, you must not give them access to /dev/mem.

> >> This is happening after newly introduced serror handling framework
> >> change which panics kernel on any any serror without checking
> >> processor mode.`
> >
> > This is the expected behaviour. The processor mode is not relevant,
> > because Serror is asynchronous -- so we cannot attribute it to userspace
> > instructions.
> >
> We recently moved to 4.17 version but if my understanding is correct
> till  Kernel 4.14, same access was resulting in bad mode and due to
> user process initiation,
> kernel was throwing exception and devmem was killed but  not
> resulting in kernel panic.

... indeed, and this was a bug, which was addressed by ensuring that
an uncontainable SError was always fatal.

> >> Kernel panic is fixed by checking processor mode in serror handler.
> >> On kernel mode, normal kernel panic action is taken and system halts.
> >> On user mode, only user process is killed and further panic action is
> >> not initiated.
> >
> > This is not safe.
> >
> > For example, an Serror could result from the kernel page tables being
> > programmed to point at device memory. A TLB walk for TTBR1 made while
> > userspace was executing could result in an SError, and killing userspace
> > alone is insufficient to contain the error.
> >
> Yes. Serror exception just gives indication and needs to be further
> analyzed but here case is direct and known and can be handled.

As abovem, the kernel does not have enough information to determine the
cause. It cannot distinguish your case from the one I described, and the
only sane thing to do in this case is to panic().

Thanks,
Mark.

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Hari Vyas]

On Tue, Jul 17, 2018 at 5:18 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 17, 2018 at 05:02:04PM +0530, Hari Vyas wrote:
>> On Tue, Jul 17, 2018 at 3:36 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Tue, Jul 17, 2018 at 03:01:21PM +0530, Hari Vyas wrote:
>> >> On executing simple user level "devmem 0x0" command, kernel panics.
>> >> As 0x0 address is mostly not matched to any valid memory so exception
>> >> is expected and raised which results in unconditional kernel panic
>> >> by serror handler.
>> >
>> > Having access to /dev/mem means that userspace can bring down the system
>> > in any number of ways.
>> >
>> > Why did userspace do this, and why do you think this shouldn't be fatal?
>> >
>> I understand,  here /dev/mem is accessible but access is issued by
>> user application devmem (which user can even do by mistake or
>> deliberately) and
>> don't think system/kernel should be  penalized for such type of minor mistake.
>
> Sorry, but this is simply a risk of exposing /dev/mem to userspace.
>
> The user could also use devmem to poke devices in ways which could
> permanently damage them. If you cannot trust the user to not do such
> things, you must not give them access to /dev/mem.
>
Okay. Don't think it is a question of trust. If access happens from
kernel mode, I understand but If user mode initiated
access(from devmem(which is just an example) or any other application)
into outside or invalid region of system
address brings complete kernel down, at least I will be surprised.
If all other also think kernel panic okay in this case, then no
further action is required from my side.
>> >> This is happening after newly introduced serror handling framework
>> >> change which panics kernel on any any serror without checking
>> >> processor mode.`
>> >
>> > This is the expected behaviour. The processor mode is not relevant,
>> > because Serror is asynchronous -- so we cannot attribute it to userspace
>> > instructions.
>> >
>> We recently moved to 4.17 version but if my understanding is correct
>> till  Kernel 4.14, same access was resulting in bad mode and due to
>> user process initiation,
>> kernel was throwing exception and devmem was killed but  not
>> resulting in kernel panic.
>
> ... indeed, and this was a bug, which was addressed by ensuring that
> an uncontainable SError was always fatal.
>
If I check commit message for do_serror(), it says some hooks can be
added to avoid panic.
---
commit a92d4d1454ab8b43b80b89fa31fcedb8821f8164
Author: Xie XiuQi <xiexiuqi@huawei.com>

Future patches may change do_serror() to return if the SError
Interrupt was notification of a
corrected error
 --

>> >> Kernel panic is fixed by checking processor mode in serror handler.
>> >> On kernel mode, normal kernel panic action is taken and system halts.
>> >> On user mode, only user process is killed and further panic action is
>> >> not initiated.
>> >
>> > This is not safe.
>> >
>> > For example, an Serror could result from the kernel page tables being
>> > programmed to point at device memory. A TLB walk for TTBR1 made while
>> > userspace was executing could result in an SError, and killing userspace
>> > alone is insufficient to contain the error.
>> >
>> Yes. Serror exception just gives indication and needs to be further
>> analyzed but here case is direct and known and can be handled.
>
> As abovem, the kernel does not have enough information to determine the
> cause. It cannot distinguish your case from the one I described, and the
> only sane thing to do in this case is to panic().
>
Believe notification and if possible corrective action is important and.
As stated, if all also think that kernel should panic in this situation,
issue can be closed. No issue from my end.

Regards,
hari
> Thanks,
> Mark.

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[James Morse]

Hi hari,

On 17/07/18 14:40, Hari Vyas wrote:
> On Tue, Jul 17, 2018 at 5:18 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> ... indeed, and this was a bug, which was addressed by ensuring that
>> an uncontainable SError was always fatal.
>>
> If I check commit message for do_serror(), it says some hooks can be
> added to avoid panic.
> ---
> commit a92d4d1454ab8b43b80b89fa31fcedb8821f8164
> Author: Xie XiuQi <xiexiuqi@huawei.com>
> 
> Future patches may change do_serror() to return if the SError
> Interrupt was notification of a
> corrected error
>  --

This is referring to the v8.2 RAS Extensions, which add architect-ed ESR values
to SError. In this example the CPU would use the ESR_ELx.AET bits to describe
the error as something hardware has corrected. e.g. using ecc. (details in
D10.2.39 of the ARM-ARM and [0])
commit 6bf0dcfd7135 ("arm64: kernel: Survive corrected RAS errors notified by
SError") adds these hooks based on the ESR information.

Without the information in the ESR (and a few other registers), we can't know
what the SError means, it has to be fatal.


Thanks,

James

[0]
https://static.docs.arm.com/ddi0587/a/RAS%20Extension-release%20candidate_march_29.pdf

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Robin Murphy]

On 17/07/18 14:40, Hari Vyas wrote:
[...]
>> Sorry, but this is simply a risk of exposing /dev/mem to userspace.
>>
>> The user could also use devmem to poke devices in ways which could
>> permanently damage them. If you cannot trust the user to not do such
>> things, you must not give them access to /dev/mem.
>>
> Okay. Don't think it is a question of trust. If access happens from
> kernel mode, I understand but If user mode initiated
> access(from devmem(which is just an example) or any other application)
> into outside or invalid region of system
> address brings complete kernel down, at least I will be surprised.

If you have an Arm Juno board, why not try this fun little demonstration 
I once wrote for a colleague:


#include <fcntl.h>
#include <sys/mman.h>

int main(void) {
	int *dmc, fd = open("/dev/mem", O_RDWR|O_SYNC);
	
	dmc = mmap(0, 4096, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0x2b0a0000);
	dmc[2] = 1;
}


The simple fact is that anyone with access to /dev/mem *can* take down 
the system any number of ways. It's foolish to think there's any way of 
protecting against that other than by not giving them access at all.

Robin.


[ For anyone interested, it puts the DRAM controller into sleep mode. 
The kernel can't even panic if all the memory suddenly disappears :D ]

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Russell King - ARM Linux]

On Tue, Jul 17, 2018 at 07:10:25PM +0530, Hari Vyas wrote:
> Okay. Don't think it is a question of trust. If access happens from
> kernel mode, I understand but If user mode initiated
> access(from devmem(which is just an example) or any other application)
> into outside or invalid region of system
> address brings complete kernel down, at least I will be surprised.
> If all other also think kernel panic okay in this case, then no
> further action is required from my side.

There are many, many ways that someone with privileged access can take
the system down.

It's _well_ known that accesses through /dev/mem (which gives _direct_
access to the hardware in the system) has the capability to take the
system down.

Your example may be through an exception, but another way could be to
find the clock controller and disable system clocks, or find the RAM
controller in physical memory and disable that.  Or the L2 cache in
older ARMs and tell it to invalidate all its contents, leading to
system data corruption and probably a kernel panic.

Other ways (on x86) that have been known for years include configuring
an 8250 serial port at an address or interrupt (this doesn't involve
/dev/mem, but merely the setserial program.)

Another, non-hardware way is the killall program used from a privileged
context.  On some flavours of Unix, it terminates programs that match
the argument.  On others, it terminates _all_ programs without warning,
effectively taking the system down.

There are 32-bit ARM SoCs that hard-lock if you try to access a device
and the clocks for that device are disabled - irrespective of whether
that is a kernel or user mode access.

Whoever has privileged system access (iow, the ability to directly
talk to hardware or configure drivers) has a responsibility to know
what to do and, more importantly, what not to do.  Poking around in
/dev/mem definitely comes under the heading of "don't do that unless
you know *exactly* what you are doing".

In normal circumstances, _nothing_ in userspace should have /dev/mem
open - indeed, the kernel build system provides an option to disable
this special device file via the CONFIG_DEVMEM.

It has to be said that /dev/mem should _never_ be exposed to non-
privileged users on the system - it's way too dangerous to do so,
since as soon as you do that, you have *no* system security what so
ever.  Your average user is then at liberty to manually read *and*
*write* the hardware *including* the kernel code _and_ page tables,
which means they can bypass *all* software protections and some
hardware ones as well.

TBH, I'd say that for people who don't know what they're doing, poking
about in /dev/mem is like pointing a gun at your foot while squeezing
the trigger.  It won't end well.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[PATCH] arm64: fix kernel panic on serror exception caused by user process

[Hari Vyas]

Hi Russell and all,

On Tue, Jul 17, 2018 at 8:15 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Jul 17, 2018 at 07:10:25PM +0530, Hari Vyas wrote:
>> Okay. Don't think it is a question of trust. If access happens from
>> kernel mode, I understand but If user mode initiated
>> access(from devmem(which is just an example) or any other application)
>> into outside or invalid region of system
>> address brings complete kernel down, at least I will be surprised.
>> If all other also think kernel panic okay in this case, then no
>> further action is required from my side.
>
> There are many, many ways that someone with privileged access can take
> the system down.
>
> It's _well_ known that accesses through /dev/mem (which gives _direct_
> access to the hardware in the system) has the capability to take the
> system down.
>
> Your example may be through an exception, but another way could be to
> find the clock controller and disable system clocks, or find the RAM
> controller in physical memory and disable that.  Or the L2 cache in
> older ARMs and tell it to invalidate all its contents, leading to
> system data corruption and probably a kernel panic.
>
> Other ways (on x86) that have been known for years include configuring
> an 8250 serial port at an address or interrupt (this doesn't involve
> /dev/mem, but merely the setserial program.)
>
> Another, non-hardware way is the killall program used from a privileged
> context.  On some flavours of Unix, it terminates programs that match
> the argument.  On others, it terminates _all_ programs without warning,
> effectively taking the system down.
>
> There are 32-bit ARM SoCs that hard-lock if you try to access a device
> and the clocks for that device are disabled - irrespective of whether
> that is a kernel or user mode access.
>
> Whoever has privileged system access (iow, the ability to directly
> talk to hardware or configure drivers) has a responsibility to know
> what to do and, more importantly, what not to do.  Poking around in
> /dev/mem definitely comes under the heading of "don't do that unless
> you know *exactly* what you are doing".
>
> In normal circumstances, _nothing_ in userspace should have /dev/mem
> open - indeed, the kernel build system provides an option to disable
> this special device file via the CONFIG_DEVMEM.
>
> It has to be said that /dev/mem should _never_ be exposed to non-
> privileged users on the system - it's way too dangerous to do so,
> since as soon as you do that, you have *no* system security what so
> ever.  Your average user is then at liberty to manually read *and*
> *write* the hardware *including* the kernel code _and_ page tables,
> which means they can bypass *all* software protections and some
> hardware ones as well.
>
> TBH, I'd say that for people who don't know what they're doing, poking
> about in /dev/mem is like pointing a gun at your foot while squeezing
> the trigger.  It won't end well.
>
Thanks for clarifying concern. My idea was to keep kernel running no
matter what user does
and due to that I proposed simple fix to avoid panic with some
notification(which worked in our
environment) and was as per little bit old kernel 4.14 version.

All suggested example modifies system/device memory
i.e.clock,dram,uart so user knows very well
what is being done and so may expect weird behavior but here I was
just (read)accessing i.e.
 not modifying. I understand (read) access could also result in
security violation and it will be difficult to
distinguish case and  if all are thinking that it is expected behavior
and best to panic kernel, I am also Okay.

> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up