From: f.fainelli@gmail•com (Florian Fainelli)
To: linux-arm-kernel@lists•infradead.org
Subject: Executable mapping of on-chip registers through /dev/mem?
Date: Thu, 19 Nov 2015 11:36:20 -0800 [thread overview]
Message-ID: <564E24B4.1080302@gmail.com> (raw)
In-Reply-To: <20151119151728.GZ8644@n2100.arm.linux.org.uk>
On 19/11/15 07:17, Russell King - ARM Linux wrote:
> On Wed, Nov 18, 2015 at 10:21:06AM -0800, Florian Fainelli wrote:
>> It turns out, that we can re-create that condition just that by opening
>> /dev/mem and calling mmap() with PROT_EXEC, giving the physical base
>> address of the register range (0xF000_0000 typically on these
>> platforms), and a mapping size which spans the entire register range
>> (32MB), although smaller mapping size also exhibit the problem, just a
>> little slower.
>
> There's two ways of looking at this:
> 1. the kernel should protect against it and prevent you creating an
> executable /dev/mem mapping
> 2. only root can create these mappings (provided the permissions are
> set sanely on /dev/mem) and this is just another way for root to
> hang themselves.
>
> Someone may have valid reasons to be able to open /dev/mem, map it
> executable, and execute code there - for example, an expansion ROM,
> some platform specific code, etc. So I'd be very nervous about
> changing the behaviour and causing userspace regressions.
>
> In my mind, it's a case of "if it hurts when I do X then don't do X".
>
>> Tracing through the calls from drivers/char/mem.c, we have this:
>>
>> drivers/char/mem.c:
>> mmap_mem()
>> ARM does define __HAVE_PHYS_MEM_ACCESS_PROT and we have
>> CONFIG_MEM_DMA_BUFFERABLE=y for our V7 builds here
>>
>> arch/arm/mm/mmu.c:
>> -> phys_mem_access_prot()
>> -> !pfn_valid(pfn) is true
>> -> pgprot_uncached()
>>
>> If I do change the pgprot value to also include the XN bit, this problem
>> never occurs, because we satisfy the piece of hardware checking for the
>> executable bit (or lack, thereof) in the mapping.
>
> Yes, but you're changing the permissions that _any_ pgprot_uncached()
> mapping gets to be different from what the user requested. At best,
> if we're saying we want to deny executable mappings of /dev/mem, we
> should return an error if userspace tries to request such a mapping.
Well, what I ended-up doing was directly using __pgprot_modify() locally
in phys_mem_access_prot() when !pfn_valid() is true, but that is still
too broad, not changing the definition of pgprot_uncached(), precisely
as this would break other areas.
We do not want to deny executable mappings through /dev/mem for the
entire range the mapping is established, just if it falls within a
platform-specific problematic region. The more I think about it, and the
more I think we might be able to solve this by simply documenting this
as a caveat/feature of the platform rather than looking for something
overly complicated involving adding machine-specific register space
knowledge and acting upon this.
>
> However, there's an issue here which is not obvious: when you don't
> have an XN bit, then the kernel has assumptions that when you request
> read but without execute permission, you'll end up with read _and_
> execute permission. In other words, on older CPUs, even if you
> request in userspace a PROT_READ mapping, the kernel will silently
> translate that to PROT_READ | PROT_EXEC internally. So denying a
> PROT_EXEC mapping will result in /dev/mem being useless on older CPUs
> as you'd never be able to create any mapping of it.
I see, we clearly do not want that, I agree.
>
>> What is is not really clear to me, is whether we are creating a new
>> mapping of this 32MB register range on this SoC, with an uncached
>> mapping + executable bit set, or we are modifying the existing mapping
>> in that case?
>
> You're creating a new mapping in the userspace address range, it's not
> a new mapping in kernel space.
>
>> - having a better way to determine if the pfn falls within existing
>> register mappings? But without a map_io() or putting that information in
>> Device Tree, how am I sure this is an exhaustive range?
>
> Is there really some problem with having userspace avoid using
> PROT_EXEC when mapping /dev/mem, which all round seems to be the
> correct solution here ?
Well, no and yes. No because, this clearly is a simple fix, that can be
documented as such. Yes in the sense that this particular platform is
sensitive to such a thing, and can lead to particularly difficult
debugging exercises with the platform users. As I just learned, some
other people decided that open-coding a /dev/mem like was a brilliant
idea, which sort of motivated me for looking into a kernel-level
solution to stop that from happening.
Appreciate your answer, thanks!
--
Florian
prev parent reply other threads:[~2015-11-19 19:36 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-18 18:21 Executable mapping of on-chip registers through /dev/mem? Florian Fainelli
2015-11-19 15:17 ` Russell King - ARM Linux
2015-11-19 19:36 ` Florian Fainelli [this message]
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=564E24B4.1080302@gmail.com \
--to=f.fainelli@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox