All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@armlinux.org.uk>
To: Dave Gerlach <d-gerlach@ti.com>, Kees Cook <keescook@chromium.org>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Dan Williams <dan.j.williams@intel.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Shawn Guo <shawnguo@kernel.org>, Tony Lindgren <tony@atomide.com>,
	Alexandre Belloni <alexandre.belloni@free-electrons.com>,
	Nishanth Menon <nm@ti.com>
Subject: Re: [PATCH 0/3] Add memremap executable mapping and extend drivers/misc/sram.c
Date: Mon, 7 Nov 2016 11:04:44 +0000	[thread overview]
Message-ID: <20161107110444.GA23750@n2100.armlinux.org.uk> (raw)
In-Reply-To: <20161027185612.22362-1-d-gerlach@ti.com>

On Thu, Oct 27, 2016 at 01:56:09PM -0500, Dave Gerlach wrote:
> There are several instances when one would want to execute out of on-chip
> SRAM, such as PM code on ARM platforms, so once again revisiting this
> series to allow that in a generic manner. Seems that having a solution for
> allowing SRAM to be mapped as executable will help clean up PM code on several
> ARM platforms that are using ARM internal __arm_ioremap_exec API
> and also open the door for PM support on new platforms like TI AM335x and
> AM437x. This was last sent as RFC here [1] and based on comments from Russell
> King and Arnd Bergmann has been rewritten to use memremap API rather than
> ioremap API, as executable iomem does not really make sense.

This is better, as it avoids the issue that I pointed out last time
around, but I'm still left wondering about the approach.

Sure, having executable SRAM mappings sounds nice and easy, but we're
creating WX mappings.  Folk have spent a while improving the security of
the kernel by ensuring that there are no WX mappings, and this series
reintroduces them.  The sad thing is that any WX mapping which appears
at a known address can be exploited.

"A known address" can be something that appears to be random, but ends
up being the same across the same device type... or can be discovered
by some means.  Eg, consider if the WX mapping is dynamically allocated,
but occurs at exactly the same point at boot - and if this happens with
android phones, consider how many of those are out there.  Or if the
address of the WX mapping is available via some hardware register.
Or...

See Kees Cook's slides at last years kernel summit -
	https://outflux.net/slides/2015/ks/security.pdf

So, I think avoiding WX mappings - mappings should be either W or X but
not both simultaneously (see page 19.)

I guess what I'm angling at is that we don't want memremap_exec(), but
we need an API which changes the permissions of a SRAM mapping between
allowing writes and allowing execution.

-- 
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.

WARNING: multiple messages have this Message-ID (diff)
From: linux@armlinux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 0/3] Add memremap executable mapping and extend drivers/misc/sram.c
Date: Mon, 7 Nov 2016 11:04:44 +0000	[thread overview]
Message-ID: <20161107110444.GA23750@n2100.armlinux.org.uk> (raw)
In-Reply-To: <20161027185612.22362-1-d-gerlach@ti.com>

On Thu, Oct 27, 2016 at 01:56:09PM -0500, Dave Gerlach wrote:
> There are several instances when one would want to execute out of on-chip
> SRAM, such as PM code on ARM platforms, so once again revisiting this
> series to allow that in a generic manner. Seems that having a solution for
> allowing SRAM to be mapped as executable will help clean up PM code on several
> ARM platforms that are using ARM internal __arm_ioremap_exec API
> and also open the door for PM support on new platforms like TI AM335x and
> AM437x. This was last sent as RFC here [1] and based on comments from Russell
> King and Arnd Bergmann has been rewritten to use memremap API rather than
> ioremap API, as executable iomem does not really make sense.

This is better, as it avoids the issue that I pointed out last time
around, but I'm still left wondering about the approach.

Sure, having executable SRAM mappings sounds nice and easy, but we're
creating WX mappings.  Folk have spent a while improving the security of
the kernel by ensuring that there are no WX mappings, and this series
reintroduces them.  The sad thing is that any WX mapping which appears
at a known address can be exploited.

"A known address" can be something that appears to be random, but ends
up being the same across the same device type... or can be discovered
by some means.  Eg, consider if the WX mapping is dynamically allocated,
but occurs at exactly the same point at boot - and if this happens with
android phones, consider how many of those are out there.  Or if the
address of the WX mapping is available via some hardware register.
Or...

See Kees Cook's slides at last years kernel summit -
	https://outflux.net/slides/2015/ks/security.pdf

So, I think avoiding WX mappings - mappings should be either W or X but
not both simultaneously (see page 19.)

I guess what I'm angling at is that we don't want memremap_exec(), but
we need an API which changes the permissions of a SRAM mapping between
allowing writes and allowing execution.

-- 
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.

  parent reply	other threads:[~2016-11-07 11:04 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-27 18:56 [PATCH 0/3] Add memremap executable mapping and extend drivers/misc/sram.c Dave Gerlach
2016-10-27 18:56 ` Dave Gerlach
2016-10-27 18:56 ` Dave Gerlach
2016-10-27 18:56 ` [PATCH 1/3] ARM: memremap: implement arch_memremap_exec/exec_nocache Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-10-27 18:56 ` [PATCH 2/3] memremap: add MEMREMAP_EXEC and MEMREMAP_EXEC_NOCACHE flags Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-10-27 18:56 ` [PATCH 3/3] misc: SRAM: Add option to map SRAM to allow code execution Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-10-27 18:56   ` Dave Gerlach
2016-11-05  2:47 ` [PATCH 0/3] Add memremap executable mapping and extend drivers/misc/sram.c Alexandre Belloni
2016-11-05  2:47   ` Alexandre Belloni
2016-11-07 11:04 ` Russell King - ARM Linux [this message]
2016-11-07 11:04   ` Russell King - ARM Linux
2016-11-07 17:43   ` Tony Lindgren
2016-11-07 17:43     ` Tony Lindgren
2016-11-07 21:34     ` Dave Gerlach
2016-11-07 21:34       ` Dave Gerlach
2016-11-07 21:34       ` Dave Gerlach

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161107110444.GA23750@n2100.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=alexandre.belloni@free-electrons.com \
    --cc=arnd@arndb.de \
    --cc=d-gerlach@ti.com \
    --cc=dan.j.williams@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=shawnguo@kernel.org \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.