Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1459442 > unrolled thread

Re: [PATCH 2/2] arm: apply more __ro_after_init

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] arm: apply more __ro_after_init Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-10 20:30 +0200
    Re: [PATCH 2/2] arm: apply more __ro_after_init Arnd Bergmann <arnd@arndb.de> - 2016-08-10 21:50 +0200
      Re: [PATCH 2/2] arm: apply more __ro_after_init Kees Cook <keescook@chromium.org> - 2016-08-10 23:50 +0200
      Re: [PATCH 2/2] arm: apply more __ro_after_init Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-11 01:10 +0200
        Re: [PATCH 2/2] arm: apply more __ro_after_init Arnd Bergmann <arnd@arndb.de> - 2016-08-11 18:00 +0200
          Re: [PATCH 2/2] arm: apply more __ro_after_init Kees Cook <keescook@chromium.org> - 2016-08-12 00:20 +0200
          Re: [PATCH 2/2] arm: apply more __ro_after_init Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-12 18:30 +0200
    Re: [PATCH 2/2] arm: apply more __ro_after_init Kees Cook <keescook@chromium.org> - 2016-08-10 22:10 +0200
    Re: [PATCH 2/2] arm: apply more __ro_after_init Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-10 22:30 +0200
    Re: [PATCH 2/2] arm: apply more __ro_after_init Arnd Bergmann <arnd@arndb.de> - 2016-08-10 23:50 +0200

#1459442 — Re: [PATCH 2/2] arm: apply more __ro_after_init

On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
> @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
>   * Any other function or debugging method which may touch any device _will_
>   * crash the kernel.
>   */
> +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
>  static void __init devicemaps_init(const struct machine_desc *mdesc)
>  {
>  	struct map_desc map;
>  	unsigned long addr;
> -	void *vectors;
> -
> -	/*
> -	 * Allocate the vector page early.
> -	 */
> -	vectors = early_alloc(PAGE_SIZE * 2);

This one is not appropriate.  We _do_ write to these pages after init
for FIQ handler updates.  See set_fiq_handler().

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1459711

On Wednesday, August 10, 2016 11:32:07 AM CEST Kees Cook wrote:
> On Wed, Aug 10, 2016 at 2:43 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
> >> @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
> >>   * Any other function or debugging method which may touch any device _will_
> >>   * crash the kernel.
> >>   */
> >> +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
> >>  static void __init devicemaps_init(const struct machine_desc *mdesc)
> >>  {
> >>       struct map_desc map;
> >>       unsigned long addr;
> >> -     void *vectors;
> >> -
> >> -     /*
> >> -      * Allocate the vector page early.
> >> -      */
> >> -     vectors = early_alloc(PAGE_SIZE * 2);
> >
> > This one is not appropriate.  We _do_ write to these pages after init
> > for FIQ handler updates.  See set_fiq_handler().
> 
> Ah, interesting. I guess none of that hardware is being tested on
> linux-next.

Right. The OMAP1 Amstrad Delta is a somewhat obscure machine, and that
would be the most likely candidate to run into this.

RiscPC also has FIQ support, but I have not heard of anyone other
than Russell still using one with a modern kernel, and I doubt he
tests linux-next on it.

The s3c24xx and imx machines that could use FIQ probably don't
use it in practice, last time I checked, I didn't see any DTS file
or platform data definition in the kernel that activated that
code path.

> I'll drop that chunk and resubmit.

Good enough for now, but it may be worth revisiting this, as the
vector page might be a good target for an attack if you have a
way to overwrite a few bytes in the kernel.

Note that there are two mappings for the pages, and as Russell
mentioned, the TLS emulation writes to the other one that is
at a fixed virtual address.

It might be better to start by making the fixed mapping readonly,
as KASLR doesn't protect that one at all, and change the TLS
code accordingly.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1460018

On Wed, Aug 10, 2016 at 12:41 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, August 10, 2016 11:32:07 AM CEST Kees Cook wrote:
>> On Wed, Aug 10, 2016 at 2:43 AM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
>> >> @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
>> >>   * Any other function or debugging method which may touch any device _will_
>> >>   * crash the kernel.
>> >>   */
>> >> +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
>> >>  static void __init devicemaps_init(const struct machine_desc *mdesc)
>> >>  {
>> >>       struct map_desc map;
>> >>       unsigned long addr;
>> >> -     void *vectors;
>> >> -
>> >> -     /*
>> >> -      * Allocate the vector page early.
>> >> -      */
>> >> -     vectors = early_alloc(PAGE_SIZE * 2);
>> >
>> > This one is not appropriate.  We _do_ write to these pages after init
>> > for FIQ handler updates.  See set_fiq_handler().
>>
>> Ah, interesting. I guess none of that hardware is being tested on
>> linux-next.
>
> Right. The OMAP1 Amstrad Delta is a somewhat obscure machine, and that
> would be the most likely candidate to run into this.
>
> RiscPC also has FIQ support, but I have not heard of anyone other
> than Russell still using one with a modern kernel, and I doubt he
> tests linux-next on it.
>
> The s3c24xx and imx machines that could use FIQ probably don't
> use it in practice, last time I checked, I didn't see any DTS file
> or platform data definition in the kernel that activated that
> code path.
>
>> I'll drop that chunk and resubmit.
>
> Good enough for now, but it may be worth revisiting this, as the
> vector page might be a good target for an attack if you have a
> way to overwrite a few bytes in the kernel.
>
> Note that there are two mappings for the pages, and as Russell
> mentioned, the TLS emulation writes to the other one that is
> at a fixed virtual address.
>
> It might be better to start by making the fixed mapping readonly,
> as KASLR doesn't protect that one at all, and change the TLS
> code accordingly.

That sounds good (anyone want to work on this?). Does arm64 need a
similar vector page protection?

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1460088

On Wed, Aug 10, 2016 at 09:41:23PM +0200, Arnd Bergmann wrote:
> It might be better to start by making the fixed mapping readonly,
> as KASLR doesn't protect that one at all, and change the TLS
> code accordingly.

I think that's impossible, because we gave userspace permission to
read 0xffff0ff0 directly without using __kuser_get_tls.  You're
talking about potentially breaking userspace.

If you disable kuser helpers, then the page becomes read-only and
invisible to userspace anyway.  So, everything is being done there
which can be done - if you have kuser helpers enabled, then you
lose some opportunities for these security improvements.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1460638

On Thursday, August 11, 2016 12:06:45 AM CEST Russell King - ARM Linux wrote:
> On Wed, Aug 10, 2016 at 09:41:23PM +0200, Arnd Bergmann wrote:
> > It might be better to start by making the fixed mapping readonly,
> > as KASLR doesn't protect that one at all, and change the TLS
> > code accordingly.
> 
> I think that's impossible, because we gave userspace permission to
> read 0xffff0ff0 directly without using __kuser_get_tls.  You're
> talking about potentially breaking userspace.
> 
> If you disable kuser helpers, then the page becomes read-only and
> invisible to userspace anyway.  So, everything is being done there
> which can be done - if you have kuser helpers enabled, then you
> lose some opportunities for these security improvements.

What I meant was writing to the page through the linear mapping
rather than the virtual mapping at 0xffff0000 so we can leave that
one read-only (I did not consider whether that might cause cache
aliasing problems when reading from the other address).

Your other point is more important though: if one really cares
about optimizing security here, they probably should disable
kuser helpers completely anyway.

Kees, is that something you have on your radar already?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1460833

On Thu, Aug 11, 2016 at 8:54 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday, August 11, 2016 12:06:45 AM CEST Russell King - ARM Linux wrote:
>> On Wed, Aug 10, 2016 at 09:41:23PM +0200, Arnd Bergmann wrote:
>> > It might be better to start by making the fixed mapping readonly,
>> > as KASLR doesn't protect that one at all, and change the TLS
>> > code accordingly.
>>
>> I think that's impossible, because we gave userspace permission to
>> read 0xffff0ff0 directly without using __kuser_get_tls.  You're
>> talking about potentially breaking userspace.
>>
>> If you disable kuser helpers, then the page becomes read-only and
>> invisible to userspace anyway.  So, everything is being done there
>> which can be done - if you have kuser helpers enabled, then you
>> lose some opportunities for these security improvements.
>
> What I meant was writing to the page through the linear mapping
> rather than the virtual mapping at 0xffff0000 so we can leave that
> one read-only (I did not consider whether that might cause cache
> aliasing problems when reading from the other address).
>
> Your other point is more important though: if one really cares
> about optimizing security here, they probably should disable
> kuser helpers completely anyway.
>
> Kees, is that something you have on your radar already?

It wasn't no. I will add it. :)

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1461307

On Thu, Aug 11, 2016 at 05:54:08PM +0200, Arnd Bergmann wrote:
> On Thursday, August 11, 2016 12:06:45 AM CEST Russell King - ARM Linux wrote:
> > On Wed, Aug 10, 2016 at 09:41:23PM +0200, Arnd Bergmann wrote:
> > > It might be better to start by making the fixed mapping readonly,
> > > as KASLR doesn't protect that one at all, and change the TLS
> > > code accordingly.
> > 
> > I think that's impossible, because we gave userspace permission to
> > read 0xffff0ff0 directly without using __kuser_get_tls.  You're
> > talking about potentially breaking userspace.
> > 
> > If you disable kuser helpers, then the page becomes read-only and
> > invisible to userspace anyway.  So, everything is being done there
> > which can be done - if you have kuser helpers enabled, then you
> > lose some opportunities for these security improvements.
> 
> What I meant was writing to the page through the linear mapping
> rather than the virtual mapping at 0xffff0000 so we can leave that
> one read-only (I did not consider whether that might cause cache
> aliasing problems when reading from the other address).

Kees original patch was about moving the vector pages into the
read-only area after init, so the linear mapping of them becomes
read-only as well.  So that won't work.  We need at least one
read-write mapping for FIQ, and for context switching for kuser
helpers.

> Your other point is more important though: if one really cares
> about optimizing security here, they probably should disable
> kuser helpers completely anyway.

We could probably predicate moving the vectors page into the RO
section when kuser helpers are enabled.

> Kees, is that something you have on your radar already?

I believe Android already disable kuser helpers as of a few years ago.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1459792

On Wed, Aug 10, 2016 at 2:43 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
>> @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
>>   * Any other function or debugging method which may touch any device _will_
>>   * crash the kernel.
>>   */
>> +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
>>  static void __init devicemaps_init(const struct machine_desc *mdesc)
>>  {
>>       struct map_desc map;
>>       unsigned long addr;
>> -     void *vectors;
>> -
>> -     /*
>> -      * Allocate the vector page early.
>> -      */
>> -     vectors = early_alloc(PAGE_SIZE * 2);
>
> This one is not appropriate.  We _do_ write to these pages after init
> for FIQ handler updates.  See set_fiq_handler().

Ah, interesting. I guess none of that hardware is being tested on
linux-next. I'll drop that chunk and resubmit.

Thanks!

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1459832

On Wed, Aug 10, 2016 at 12:00:53PM +0200, Arnd Bergmann wrote:
> On Wednesday, August 10, 2016 10:43:39 AM CEST Russell King - ARM Linux wrote:
> > On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
> > > @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
> > >   * Any other function or debugging method which may touch any device _will_
> > >   * crash the kernel.
> > >   */
> > > +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
> > >  static void __init devicemaps_init(const struct machine_desc *mdesc)
> > >  {
> > >       struct map_desc map;
> > >       unsigned long addr;
> > > -     void *vectors;
> > > -
> > > -     /*
> > > -      * Allocate the vector page early.
> > > -      */
> > > -     vectors = early_alloc(PAGE_SIZE * 2);
> > 
> > This one is not appropriate.  We _do_ write to these pages after init
> > for FIQ handler updates.  See set_fiq_handler().
> 
> Is that the only thing that modifies the page? If we think this is a
> valuable change, we could make it depend on the absence of FIQ
> support, as very few platforms (rpc, omap1, s3c24xx and possibly
> imx) seem to even use it.

There's the TLS emulation too, but that writes via the vectors mapping
at 0xffff0ff0.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1460017

On Wednesday, August 10, 2016 10:43:39 AM CEST Russell King - ARM Linux wrote:
> On Fri, Jun 03, 2016 at 11:40:24AM -0700, Kees Cook wrote:
> > @@ -1309,16 +1309,11 @@ void __init arm_mm_memblock_reserve(void)
> >   * Any other function or debugging method which may touch any device _will_
> >   * crash the kernel.
> >   */
> > +static char vectors[PAGE_SIZE * 2] __ro_after_init __aligned(PAGE_SIZE);
> >  static void __init devicemaps_init(const struct machine_desc *mdesc)
> >  {
> >       struct map_desc map;
> >       unsigned long addr;
> > -     void *vectors;
> > -
> > -     /*
> > -      * Allocate the vector page early.
> > -      */
> > -     vectors = early_alloc(PAGE_SIZE * 2);
> 
> This one is not appropriate.  We _do_ write to these pages after init
> for FIQ handler updates.  See set_fiq_handler().

Is that the only thing that modifies the page? If we think this is a
valuable change, we could make it depend on the absence of FIQ
support, as very few platforms (rpc, omap1, s3c24xx and possibly
imx) seem to even use it.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web