Message-ID: <48924BBB91ABDE4D9335632A6B179DD6A8E6B2@MIPSMAIL01.mipstec.com>
Date:   Thu, 7 Dec 2017 11:33:47 +0000
From:   Miodrag Dinic <Miodrag.Dinic@...s.com>
To:     Paul Burton <Paul.Burton@...s.com>,
        Maciej Rozycki <Maciej.Rozycki@...s.com>,
        Aleksandar Markovic <aleksandar.markovic@...rk.com>,
        Aleksandar Markovic <Aleksandar.Markovic@...s.com>
CC:     James Hogan <James.Hogan@...s.com>,
        David Daney <ddaney@...iumnetworks.com>,
        "linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        DengCheng Zhu <DengCheng.Zhu@...s.com>,
        Ding Tianhong <dingtianhong@...wei.com>,
        Douglas Leung <Douglas.Leung@...s.com>,
        "Frederic Weisbecker" <frederic@...nel.org>,
        Goran Ferenc <Goran.Ferenc@...s.com>,
        Ingo Molnar <mingo@...nel.org>,
        James Cowgill <James.Cowgill@...tec.com>,
        Jonathan Corbet <corbet@....net>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        "Matt Redfearn" <Matt.Redfearn@...s.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Petar Jovanovic <Petar.Jovanovic@...s.com>,
        Raghu Gandham <Raghu.Gandham@...s.com>,
        "Ralf Baechle" <ralf@...ux-mips.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Tom Saeger" <tom.saeger@...cle.com>
Subject: RE: [PATCH v2] MIPS: Add nonxstack=on|off kernel parameter

Hi Paul, David, Maciej,

thanks for taking interest in this topic and your valuable comments.
Aleksandar is currently on sick leave but I think I can address
all previous questions and concerns by answering to this Pauls last post:

> On Wed, Dec 06, 2017 at 05:50:52PM +0000, Maciej W. Rozycki wrote:
> >  What problem are you trying to solve anyway?  Is it not something that 
> > can be handled with the `execstack' utility?
> 
> The commit message states that for Android "non-exec stack is required".
> Is Android checking that then Aleksandar? If so, how? 

Android is using SELinux configured to disallow NX mappings by handling
the following sepolicy rules :
* Executable stack (execstack)
* Executable heap (execheap)
* File-based executable code which has been modified (execmod)
* All other executable memory (execmem)

Nice explanation can be found here:
https://android-review.googlesource.com/#/c/platform/bionic/+/145004/

> I presume what you
> actually want here is for the kernel to lie & indicate to whatever part
> of Android that performs this check that the stack is non-executable
> even when it is really executable?

Basically yes, because we do not have other options at this point.
And this kernel parameter should definitely not be used in a production
environments. Hopefully, any future MIPS Android device would have
CPU with RIXI.

> Is this aimed at the Android emulator? If so would it be possible to
> instead implement RIXI support & make the non-exec stack actually work?

This issue was indeed detected using Android emulator while testing the "Ranchu"
platform kernel we are trying to integrate in another series.

We could probably change the emulator and enable RIXI, but the whole point of using it
is to emulate real HW as close as possible (And detect issues which could be
found on real HW as well), so I would tend to keep it unmodified.
But if we do not come to a consensus regarding this patch, we would need to modify it.

For emulating android on different MIPS arch variants we are using the
following CPU emulations:
- For mips32[r1|r2][dsp] - we are using 74Kf - No RIXI
- For mips32r5[msa] - we are using P5600 - has RIXI
- For mips[32|64]r6[msa] - I6400 - has RIXI

The CI20 development board (lacks XI) would also be affected  by this issue and it has been running
Android for quite some time now. The kernel for CI20 which we are currently using
is 3.18 and it does NOT yet contain 1a770b85c1f1 ("MIPS: non-exec stack & heap when non-exec
PT_GNU_STACK is present").

The effect of not having some workaround like this in the kernel, would
be to run Android only in SELinux permissive mode.

> >  NB as someone has observed with programs that do not request a 
> > non-executable stack we actually propagate the execute permission to all 
> > data pages.  Is it not something we would want to handle differently?
> 
> It would of course be ideal to mark data/heap memory non-executable -
> the question is how should we know that it's safe to do so. The approach
> I took in 1a770b85c1f1 ("MIPS: non-exec stack & heap when non-exec
> PT_GNU_STACK is present") was to require the PT_GNU_STACK header in
> order to mark both stack & heap non-executable, for reasons outlined in
> its commit message:
> 
>   - I was told at the time that no MIPS tools were yet emitting
>     PT_GNU_STACK, so we wouldn't be changing the behaviour of any
>     existing binaries & thus wouldn't break any.
> 
>   - It matches the behaviour of both ARM & x86.
> 

For Android, Google placed PT_GNU_STACK header in crtend* and this effectively
makes all executables and libraries request NX stack:
https://android-review.googlesource.com/#/c/platform/bionic/+/40773/

The toolchain for Android was updated as well:
https://android-review.googlesource.com/#/c/platform/ndk/+/37140/

------------------

@David:

> All Cavium processors since OCTEON Plus (more than ten years ago)
> support RIXI.

Sure, I missed to mention Cavium. Than I suppose, Cavium would have no issues running Android.

> Also, this does nothing for multi-threaded programs where libc sets the
> permissions on the thread stacks.

I don't think Android bionic is doing anything similar, at least we
haven't seen any issues so far related to this.

> If you really need something, at a minimum, use the same parameter name
> that x86 uses.

Sure, I agree, this was my dilemma as well, we will update the parameter name
to be "noexec".

Kind regards,
Miodrag
________________________________________
From: Paul Burton [paul.burton@...s.com]
Sent: Wednesday, December 6, 2017 7:24 PM
To: Maciej Rozycki; Aleksandar Markovic; Aleksandar Markovic
Cc: Miodrag Dinic; James Hogan; David Daney; linux-mips@...ux-mips.org; Andrew Morton; DengCheng Zhu; Ding Tianhong; Douglas Leung; Frederic Weisbecker; Goran Ferenc; Ingo Molnar; James Cowgill; Jonathan Corbet; linux-doc@...r.kernel.org; linux-kernel@...r.kernel.org; Marc Zyngier; Matt Redfearn; Mimi Zohar; Paul E. McKenney; Petar Jovanovic; Raghu Gandham; Ralf Baechle; Thomas Gleixner; Tom Saeger
Subject: Re: [PATCH v2] MIPS: Add nonxstack=on|off kernel parameter

Hi Maciej, Aleksandar,

On Wed, Dec 06, 2017 at 05:50:52PM +0000, Maciej W. Rozycki wrote:
>  What problem are you trying to solve anyway?  Is it not something that
> can be handled with the `execstack' utility?

The commit message states that for Android "non-exec stack is required".
Is Android checking that then Aleksandar? If so, how? I presume what you
actually want here is for the kernel to lie & indicate to whatever part
of Android that performs this check that the stack is non-executable
even when it is really executable?

Is this aimed at the Android emulator? If so would it be possible to
instead implement RIXI support & make the non-exec stack actually work?

>  NB as someone has observed with programs that do not request a
> non-executable stack we actually propagate the execute permission to all
> data pages.  Is it not something we would want to handle differently?

It would of course be ideal to mark data/heap memory non-executable -
the question is how should we know that it's safe to do so. The approach
I took in 1a770b85c1f1 ("MIPS: non-exec stack & heap when non-exec
PT_GNU_STACK is present") was to require the PT_GNU_STACK header in
order to mark both stack & heap non-executable, for reasons outlined in
its commit message:

  - I was told at the time that no MIPS tools were yet emitting
    PT_GNU_STACK, so we wouldn't be changing the behaviour of any
    existing binaries & thus wouldn't break any.

  - It matches the behaviour of both ARM & x86.

Marking the heap non-executable by default would have advantages in that
we wouldn't need to worry about icache coherency for it in
set_pte_at()/__update_cache(), so one idea I had was that we could
possibly initially mark pages non-executable in the TLB & later enable
execution only if we take a TLBXI exception, with the assumption being
that in most cases we'll never try executing from the heap. That's not
an idea I've yet found the time to implement or measure the impact of
though.

Thanks,
    Paul

Powered by blists - more mailing lists