Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Jann Horn]

+32-bit ARM folks

On Fri, Jun 14, 2019 at 5:10 PM Denis 'GNUtoo' Carikli
<GNUtoo@cyberdimension.org> wrote:
> On a Galaxy SIII (I9300), the patch mentioned below broke boot:
> - The display still had the bootloader logo, while with this
>   patch, the 4 Tux logo appears.
> - No print appeared on the serial port anymore after the kernel
>   was loaded, whereas with this patch, we have the serial
>   console working, and the device booting.
>
> Booting was broken by the following commit:
>   9f671e58159a ("security: Create "kernel hardening" config area")
>
> As the bootloader of this device enables the MMU, I had the following
> patch applied during the tests:
>   Author: Arve Hjønnevåg <arve@android.com>
>   Date:   Fri Nov 30 17:05:40 2012 -0800
>
>       ANDROID: arm: decompressor: Flush tlb before swiching domain 0 to client mode
>
>       If the bootloader used a page table that is incompatible with domain 0
>       in client mode, and boots with the mmu on, then swithing domain 0 to
>       client mode causes a fault if we don't flush the tlb after updating
>       the page table pointer.
>
>       v2: Add ISB before loading dacr.
>
>   diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
>   index 7135820f76d4..6e87ceda3b29 100644
>   --- a/arch/arm/boot/compressed/head.S
>   +++ b/arch/arm/boot/compressed/head.S
>   @@ -837,6 +837,8 @@ __armv7_mmu_cache_on:
>                   bic     r6, r6, #1 << 31        @ 32-bit translation system
>                   bic     r6, r6, #(7 << 0) | (1 << 4)    @ use only ttbr0
>                   mcrne   p15, 0, r3, c2, c0, 0   @ load page table pointer
>   +               mcrne   p15, 0, r0, c8, c7, 0   @ flush I,D TLBs
>   +               mcr     p15, 0, r0, c7, c5, 4   @ ISB
>                   mcrne   p15, 0, r1, c3, c0, 0   @ load domain access control
>                   mcrne   p15, 0, r6, c2, c0, 2   @ load ttb control
>    #endif
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> ---
>  scripts/gcc-plugins/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
> index e9c677a53c74..afa1db3d3471 100644
> --- a/scripts/gcc-plugins/Kconfig
> +++ b/scripts/gcc-plugins/Kconfig
> @@ -18,7 +18,6 @@ config GCC_PLUGINS
>         bool
>         depends on HAVE_GCC_PLUGINS
>         depends on PLUGIN_HOSTCC != ""
> -       default y
>         help
>           GCC plugins are loadable modules that provide extra features to the
>           compiler. They are useful for runtime instrumentation and static analysis.

I don't think GCC_PLUGINS alone is supposed to generate any code? It
just makes it possible to enable a bunch of other kconfig flags that
can generate code.

STACKPROTECTOR_PER_TASK defaults to y and depends on GCC_PLUGINS, so
is that perhaps what broke? Can you try whether disabling just that
works for you?

My guess is that maybe there is some early boot code that needs to
have the stack protector disabled, or something like that.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Denis 'GNUtoo' Carikli]

[-- Attachment #1.1: Type: text/plain, Size: 316 bytes --]

On Fri, 14 Jun 2019 18:05:19 +0200
Jann Horn <jannh@google.com> wrote:

[...]
> STACKPROTECTOR_PER_TASK defaults to y and depends on GCC_PLUGINS, so
> is that perhaps what broke? Can you try whether disabling just that
> works for you?
I've tried that, and it's sufficient to make the device boot.

Denis.

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 06:05:19PM +0200, Jann Horn wrote:
> +32-bit ARM folks
> 
> On Fri, Jun 14, 2019 at 5:10 PM Denis 'GNUtoo' Carikli
> <GNUtoo@cyberdimension.org> wrote:
> > On a Galaxy SIII (I9300), the patch mentioned below broke boot:
> > - The display still had the bootloader logo, while with this
> >   patch, the 4 Tux logo appears.
> > - No print appeared on the serial port anymore after the kernel
> >   was loaded, whereas with this patch, we have the serial
> >   console working, and the device booting.
> >
> > Booting was broken by the following commit:
> >   9f671e58159a ("security: Create "kernel hardening" config area")
> >
> > As the bootloader of this device enables the MMU, I had the following
> > patch applied during the tests:
> >   Author: Arve Hjønnevåg <arve@android.com>
> >   Date:   Fri Nov 30 17:05:40 2012 -0800
> >
> >       ANDROID: arm: decompressor: Flush tlb before swiching domain 0 to client mode
> >
> >       If the bootloader used a page table that is incompatible with domain 0
> >       in client mode, and boots with the mmu on, then swithing domain 0 to
> >       client mode causes a fault if we don't flush the tlb after updating
> >       the page table pointer.
> >
> >       v2: Add ISB before loading dacr.

I'm wondering whether this is sloppy wording or whether the author is
really implying that they call the kernel decompressor with the MMU
enabled, against the express instructions in Documentation/arm/Booting.

If they are going against the express instructions, all bets are off.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Denis 'GNUtoo' Carikli]

[-- Attachment #1.1: Type: text/plain, Size: 1452 bytes --]

On Fri, 14 Jun 2019 17:28:11 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> I'm wondering whether this is sloppy wording or whether the author is
> really implying that they call the kernel decompressor with the MMU
> enabled, against the express instructions in
> Documentation/arm/Booting.
According to [1]
> If they are going against the express instructions, all bets are off.

More background on the decompressor patch:
- The "ANDROID: arm: decompressor: Flush tlb before swiching domain 0 to
  client mode" patch is needed anyway since 3.4 in any case, and
  according to the thread about it [1], the MMU is on at boot.
- There is a downstream u-boot port for the Galaxy SIII and other very
  similar devices, which doesn't setup the MMU at boot, but I'm not
  confident enough to test in on the devices I have. To test with
  u-boot I'd need to find a new device.
- If I don't manage to find a new device to test on, since there is
  already some setup code like arch/arm/boot/compressed/head-sa1100.S
  that deal with MMU that are enabled with the bootloader, are patches
  to add a new file like that still accepted? The big downside is that
  using something like that is probably incompatible with
  ARCH_MULTIPLATFORM.

References:
-----------
[1]http://lkml.iu.edu/hypermail/linux/kernel/1212.1/02099.html
[2]https://blog.forkwhiletrue.me/posts/an-almost-fully-libre-galaxy-s3/

Denis.

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 08:14:34PM +0200, Denis 'GNUtoo' Carikli wrote:
> On Fri, 14 Jun 2019 17:28:11 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > I'm wondering whether this is sloppy wording or whether the author is
> > really implying that they call the kernel decompressor with the MMU
> > enabled, against the express instructions in
> > Documentation/arm/Booting.
> According to [1]
> > If they are going against the express instructions, all bets are off.
> 
> More background on the decompressor patch:
> - The "ANDROID: arm: decompressor: Flush tlb before swiching domain 0 to
>   client mode" patch is needed anyway since 3.4 in any case, and
>   according to the thread about it [1], the MMU is on at boot.
> - There is a downstream u-boot port for the Galaxy SIII and other very
>   similar devices, which doesn't setup the MMU at boot, but I'm not
>   confident enough to test in on the devices I have. To test with
>   u-boot I'd need to find a new device.
> - If I don't manage to find a new device to test on, since there is
>   already some setup code like arch/arm/boot/compressed/head-sa1100.S
>   that deal with MMU that are enabled with the bootloader, are patches
>   to add a new file like that still accepted? The big downside is that
>   using something like that is probably incompatible with
>   ARCH_MULTIPLATFORM.

SA11x0 pre-dates the booting document, which came about because of the
desire to make the kernel less dependent on the host CPU type.  So
"sa11x0 does it so we can do it" is really not an argument I ever want
to see to justify this kind of stuff.

The booting requirements have been known since at least 2002, some
SEVENTEEN years ago, and the problem was identified as buggy back in
2012.  As far as I can see, nothing has changed.

Entering the kernel with the MMU on and optionally caches on is an
inherently unsafe thing to do.  The kernel would have been placed into
RAM via the data cache, and then we're trying to execute code - unless
the caches have been properly cleaned and invalidated, there is no
guarantee that we'd even reach any instructions to do our own cache
cleaning and invalidation.  So, caches on is utter madness.

MMU on presents a problem: the kernel moves itself around during
decompression - if it happens to move itself on top of the in-use
page tables, then that would be really bad.  There's another issue as
well - if the page tables are already setup, and we create a different
mapping for the virtual address range, the _only_ way to safely switch
to that mapping is via a break-make arrangement, which means we need
code to disable the MMU, flush it.  It is not as simple as "a few extra
instructions to flush TLBs" although that may work in the majority of
cases.  Architecturally, it is wrong.

Things can get even worse - what if the page tables are located where
the kernel writes its own page tables - modifying the live tables and
changing the type of the entries.  Architecturally unpredictable
behaviour may result.

What is written in Documentation/arm/Booting is not for our fun, it is
there to spell out what the kernel requires to be able to boot reliably
on hardware.  If it isn't followed, then booting a kernel will be
unreliable.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Paul Kocialkowski]

Hi,

On Fri, 2019-06-14 at 20:14 +0200, Denis 'GNUtoo' Carikli wrote:
> On Fri, 14 Jun 2019 17:28:11 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > I'm wondering whether this is sloppy wording or whether the author is
> > really implying that they call the kernel decompressor with the MMU
> > enabled, against the express instructions in
> > Documentation/arm/Booting.
> According to [1]
> > If they are going against the express instructions, all bets are off.
> 
> More background on the decompressor patch:
> - The "ANDROID: arm: decompressor: Flush tlb before swiching domain 0 to
>   client mode" patch is needed anyway since 3.4 in any case, and
>   according to the thread about it [1], the MMU is on at boot.
> - There is a downstream u-boot port for the Galaxy SIII and other very
>   similar devices, which doesn't setup the MMU at boot, but I'm not
>   confident enough to test in on the devices I have. To test with
>   u-boot I'd need to find a new device.
> - If I don't manage to find a new device to test on, since there is
>   already some setup code like arch/arm/boot/compressed/head-sa1100.S
>   that deal with MMU that are enabled with the bootloader, are patches
>   to add a new file like that still accepted? The big downside is that
>   using something like that is probably incompatible with
>   ARCH_MULTIPLATFORM.

Maybe we could also consider having a shim that is executed before the
kernel in order to sanitize things and allow booting a mainline kernel,
which would be less invasive than a full U-Boot port.

Other than that, we can probably manage keeping a tree around (at the
Replicant project) with mainline and this patch (enabled through a
dedicated config option). As long as it's not horrible to rebase, it
can work well enough for us. 

I'm also not sure about the state of Android support in mainline today,
but there's a chance we'll need to pick a few patches on top of
mainline anyway.

What do you think?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Denis 'GNUtoo' Carikli]

[-- Attachment #1.1: Type: text/plain, Size: 3349 bytes --]

On Sat, 15 Jun 2019 12:13:15 +0200
Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:

> Hi,
Hi,

> Other than that, we can probably manage keeping a tree around (at the
> Replicant project) with mainline and this patch (enabled through a
> dedicated config option). As long as it's not horrible to rebase, it
> can work well enough for us.
I've managed to buy a new Galaxy SIII 4G (I9305) and I've tried u-boot
on it, and it works flawlessly without any patches and it does also
work with CONFIG_STACKPROTECTOR_PER_TASK=y.

Merely rebasing that arm decompressor patch over time should not be an
issue. However I really want to find a way to avoid having to look
again and again over time for commits that incidentally broke booting,
because, the bootloader doesn't do what it's supposed to do.

> Maybe we could also consider having a shim that is executed before the
> kernel in order to sanitize things and allow booting a mainline
> kernel, which would be less invasive than a full U-Boot port.
If I understand correctly, that isn't a solution either as it
would also be affected by the issues mentioned by Russell King.

More specifically I would need to do more research to find if the
bootloader(s) shipped on such smartphones properly cleans and
invalidates the caches before jumping to the first instruction.

Doing that research probably requires decompiling the bootloader,
which in turn would require me to get legal advise to understand if it's
possible to do it, and if so how to do it while respecting the laws
involved, and still being able to work on free and open
source bootloaders without creating issues for the projects.

Another alternative to that would be to make users use u-boot but
this is not possible either because:
- The bootloader is signed. So the bootrom checks the signature of the
  first bootloader (BL1), which in turn checks the second bootloader
  (S-Boot) in which the MMU setup probably happens. So I can't merely
  replace S-Boot like that.
- Fortunately for that system on a chip, there is at least one BL1 that
  is signed but that doesn't check subsequent signatures[1]. The issue
  is that it's not redistributable[2].

If that BL1 had not been published I would always need to use additional
patches to test the patch I send, which is very problematic in many
ways:
- The additional patches would need to be mentioned in most or all of
  the commits I send upstream.
- If not, the maintainers and readers of the patch would be unaware
  that it would require another patch on top to work.

So thanks to that, I'm at least able to test the patches I send in
Linux without requiring additional patches on top, but I'm still not
able to ship something usable to end users.

This means that the work to complete the support for the affected devices will
be way less useful, as there would be no guarantee of users still being
able to use the device with newer Linux kernels. 

Are there other (Android) smartphones affected by similar bootloader
issues? If so is it even possible to replace part of the bootloader?
Did some people found a way to deal with that kind of bootloader issue?

References:
-----------
[1]https://wiki.odroid.com/_media/en/boot.tar.gz
[2]https://github.com/hardkernel/u-boot_firmware/issues/1

Denis.

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Paul Kocialkowski]

Hi,

On Sat, 2019-06-22 at 01:42 +0200, Denis 'GNUtoo' Carikli wrote:
> On Sat, 15 Jun 2019 12:13:15 +0200
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
> > Other than that, we can probably manage keeping a tree around (at the
> > Replicant project) with mainline and this patch (enabled through a
> > dedicated config option). As long as it's not horrible to rebase, it
> > can work well enough for us.
> I've managed to buy a new Galaxy SIII 4G (I9305) and I've tried u-boot
> on it, and it works flawlessly without any patches and it does also
> work with CONFIG_STACKPROTECTOR_PER_TASK=y.

That's good to know, maybe they realized that they got it wrong later
on. Hopefully this can indicate that future models are not affected.

> Merely rebasing that arm decompressor patch over time should not be an
> issue. However I really want to find a way to avoid having to look
> again and again over time for commits that incidentally broke booting,
> because, the bootloader doesn't do what it's supposed to do.

I don't think there are many more areas where the bootloader can
misbehave to a point where it will influence Linux (of course, that's
without mention of software running in the "secure" world, which can be
totally out of control as you know).

> > Maybe we could also consider having a shim that is executed before the
> > kernel in order to sanitize things and allow booting a mainline
> > kernel, which would be less invasive than a full U-Boot port.
> If I understand correctly, that isn't a solution either as it
> would also be affected by the issues mentioned by Russell King.

It is definitely a solution, but it comes with the constraint that it
must be able to run and act as a trampoline between the bootloader and
Linux. This means that the code must be able to deal with MMU and cache
enabled.

> More specifically I would need to do more research to find if the
> bootloader(s) shipped on such smartphones properly cleans and
> invalidates the caches before jumping to the first instruction.
> 
> Doing that research probably requires decompiling the bootloader,
> which in turn would require me to get legal advise to understand if it's
> possible to do it, and if so how to do it while respecting the laws
> involved, and still being able to work on free and open
> source bootloaders without creating issues for the projects.

I would rather try to just write minimal code and make sure it
generally works. We can't really have any hard guarantee, but a program
that was shown to run over and over again without faulting is probably
good enough, since it would be very small.

> Another alternative to that would be to make users use u-boot but
> this is not possible either because:
> - The bootloader is signed. So the bootrom checks the signature of the
>   first bootloader (BL1), which in turn checks the second bootloader
>   (S-Boot) in which the MMU setup probably happens. So I can't merely
>   replace S-Boot like that.
> - Fortunately for that system on a chip, there is at least one BL1 that
>   is signed but that doesn't check subsequent signatures[1]. The issue
>   is that it's not redistributable[2].
> 
> If that BL1 had not been published I would always need to use additional
> patches to test the patch I send, which is very problematic in many
> ways:
> - The additional patches would need to be mentioned in most or all of
>   the commits I send upstream.
> - If not, the maintainers and readers of the patch would be unaware
>   that it would require another patch on top to work.
> 
> So thanks to that, I'm at least able to test the patches I send in
> Linux without requiring additional patches on top, but I'm still not
> able to ship something usable to end users.
> 
> This means that the work to complete the support for the affected devices will
> be way less useful, as there would be no guarantee of users still being
> able to use the device with newer Linux kernels. 

I agree and while a U-Boot port is desirable, it's not the easiest
solution users to bootstrap mainline Linux on the device.

> Are there other (Android) smartphones affected by similar bootloader
> issues? If so is it even possible to replace part of the bootloader?
> Did some people found a way to deal with that kind of bootloader issue?

As far as I know, the few Android devices supported by mainline Linux
don't have similar issues (e.g. OMAP phones/tablets) so the situation
probably hasn't occured much.

Cheers,

Paul

> References:
> -----------
> [1]https://wiki.odroid.com/_media/en/boot.tar.gz
> [2]https://github.com/hardkernel/u-boot_firmware/issues/1
> 
> Denis.
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] security: do not enable CONFIG_GCC_PLUGINS by default

[Kees Cook]

On Fri, Jun 14, 2019 at 06:05:19PM +0200, Jann Horn wrote:
> On Fri, Jun 14, 2019 at 5:10 PM Denis 'GNUtoo' Carikli
> <GNUtoo@cyberdimension.org> wrote:
> > Booting was broken by the following commit:
> >   9f671e58159a ("security: Create "kernel hardening" config area")
> 
> I don't think GCC_PLUGINS alone is supposed to generate any code? It
> just makes it possible to enable a bunch of other kconfig flags that
> can generate code.
> 
> STACKPROTECTOR_PER_TASK defaults to y and depends on GCC_PLUGINS, so
> is that perhaps what broke? Can you try whether disabling just that
> works for you?

Yes, this has come up before: the option you want to disable is as Jann
mentions: CONFIG_STACKPROTECTOR_PER_TASK.

> My guess is that maybe there is some early boot code that needs to
> have the stack protector disabled, or something like that.

Right, though I'm not sure what portion would be specific to that
device. You can turn off SSP on a per-file basis with:

CFLAGS_target.o += $(DISABLE_ARM_SSP_PER_TASK_PLUGIN)

or per-Makefile, as in arch/arm/boot/compressed/Makefile.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel