I'm working on adding memory protection key support to glibc.
I don't think pkey_free, as it is implemented today, is very safe due to
key reuse by a subsequent pkey_alloc. I see two problems:
(A) pkey_free allows reuse for they key while there are still mappings
that use it.
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Issue (A) could be fixed by having pkey_free to mark the key for reuse,
and only actually reuse it if all those mappings are gone. This could
have a significant performance cost, but pkey_free is supposed to be rare.
Issue (B) is much harder to fix. There is no atomic way to change
access for a single key, so there is always a race condition due to the
read-modify-write cycle for the PKRU update in user space. This means
that even if the kernel iterated over all threads to revoke access on
pkey_free, there is a chance that the race reinstantiates the old access
rights.
One way to deal with this is to give up and just remove pkey_free from
the API (i.e., we wouldn't provide it in glibc). A slightly less
drastic way could add two pkey_alloc flags, a flag to disable pkey_free
for the new key (which would mainly serve as a documentation of intent),
and another flag which requests a pristine key which has never been used
before. With the second flag, and assuming correct key management,
libraries would have some confidence that other threads in the process
would not implicitly gain access to the new key (although there is the
init_pkru= boot flag, which overrides the thread default, so it doesn't
look like the assumption is actually valid).
All this is of course a bit on thin ice anyway because code could just
clear the PKRU register at any time.
I'm attaching my glibc patch for reference. The interesting bits is
probably the test case (and how it creates and joins threads) and the
pkey_set/pkey_get functions. The support/ subdirectory is just our
testing framework which is still very young—I needed a few more
functions for debugging, which is why they are in this patch.
Key reuse is not the only problem, we also have an issue with siglongjmp:
https://sourceware.org/bugzilla/show_bug.cgi?id=22396
I've started wondering whether it even makes sense to expose this
interface for general use. I don't think any other architecture will
implement something like this in the same way (with a PKRU register
which can simply be cleared, and keys which are easily guessed and
reused). I suspect the only use for this functionality is in-memory
databases which use DAX mappings for persistence, and want to reduce
risk of persistent data corruption due to random pointer writes. (And
maybe execute-only memory, but that's not really benefiting anyone anyway.)
Thanks,
Florian
PS: The manpages need fixing. Right now, they are misleading.
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-08 20:41:32
On 11/05/2017 02:35 AM, Florian Weimer wrote:
I don't think pkey_free, as it is implemented today, is very safe due to
key reuse by a subsequent pkey_alloc. I see two problems:
(A) pkey_free allows reuse for they key while there are still mappings
that use it.
I don't agree with this assessment. Is malloc() unsafe? If someone
free()s memory that is still in use, a subsequent malloc() would hand
the address out again for reuse.
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Where does this expectation come from? Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.
We define free() as only being called on resources to which there are no
active references. If you free() things in use, bad things happen.
pkey_free() is only to be called when there is nothing actively using
the key. If you pkey_free() an in-use key, bad things happen.
I don't think pkey_free, as it is implemented today, is very safe due to
key reuse by a subsequent pkey_alloc. I see two problems:
(A) pkey_free allows reuse for they key while there are still mappings
that use it.
I don't agree with this assessment. Is malloc() unsafe? If someone
free()s memory that is still in use, a subsequent malloc() would hand
the address out again for reuse.
I think the disagreement is not about what is considered acceptable
behavior as such, but what constitutes “use”.
And even if with concurrent use, the behavior can be well-defined. We
make sure that if munmap is called, we do not return before all threads
have observed in principle that the page is gone (at considerable cost,
of course, and in most cases, that is total overkill).
I'm pretty sure there is another key reuse scenario which does not even
involve pkey_free, but I need to write a test first.
quoted
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Where does this expectation come from?
For me, it was the access_rights argument to pkey_alloc. What else
would it do? For the current thread, I can already set the rights with
a PKRU write, so the existence of the syscall argument is puzzling.
Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.
But malloc/free isn't expected to be a partial antidote to random
pointer scribbling.
We define free() as only being called on resources to which there are no
active references. If you free() things in use, bad things happen.
pkey_free() is only to be called when there is nothing actively using
the key. If you pkey_free() an in-use key, bad things happen.
My impression was that MPK was intended as a fallback in case you did
that, and unrelated code suddenly writes through a dangling pointer and
accidentally hits the DAX-mapped persistent memory of the database. To
prevent that, the those pages are mapped write-disabled on all threads
almost all the time, and only if the database needs to write something,
it temporarily tweaks PKRU so that it gains access. All that assumes
that you can actually restrict all threads in the process, but with the
current implementation, that's not true even if threads never touch keys
they don't know.
I think we should either implement revoke on pkey_alloc, with a
broadcast to all threads (the pkey_set race can be closed by having a
vDSO for that an the revocation code can check %rip to see if the old
PKRU value needs to be fixed up). Or we add the two pkey_alloc flags I
mentioned earlier.
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-09 16:59:59
On 11/09/2017 06:48 AM, Florian Weimer wrote:
On 11/08/2017 09:41 PM, Dave Hansen wrote:
quoted
quoted
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Where does this expectation come from?
For me, it was the access_rights argument to pkey_alloc. What else
would it do? For the current thread, I can already set the rights with
a PKRU write, so the existence of the syscall argument is puzzling.
The manpage is pretty bare here. But the thought was that, in most
cases, you will want to allocate a key and start using it immediately.
This was in response to some feedback on one of the earlier reviews of
the patch set.
quoted
Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.
But malloc/free isn't expected to be a partial antidote to random
pointer scribbling.
Nor is protection keys intended to be an antidote for use-after-free.
I think we should either implement revoke on pkey_alloc, with a
broadcast to all threads (the pkey_set race can be closed by having a
vDSO for that an the revocation code can check %rip to see if the old
PKRU value needs to be fixed up). Or we add the two pkey_alloc flags I
mentioned earlier.
That sounds awfully complicated to put in-kernel. I'd be happy to
review the patches after you put them together once we see how it looks.
You basically want threads to broadcast their PKRU values at pkey_free()
time. That's totally doable... in userspace. You just need a mechanism
for each thread to periodically check if they need an update. I don't
think we need kernel intervention and vDSO magic for that.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
Where does this expectation come from?
For me, it was the access_rights argument to pkey_alloc. What else
would it do? For the current thread, I can already set the rights with
a PKRU write, so the existence of the syscall argument is puzzling.
The manpage is pretty bare here. But the thought was that, in most
cases, you will want to allocate a key and start using it immediately.
This was in response to some feedback on one of the earlier reviews of
the patch set.
Okay. In the future, may want to use this access rights to specify the
default for the signal handler (with a new pkey_alloc flag). If I can
the default access rights, that would pretty much solve the sigsetjmp
problem for me, too, and I can start using protection keys in low-level
glibc code.
quoted
quoted
Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.
But malloc/free isn't expected to be a partial antidote to random
pointer scribbling.
Nor is protection keys intended to be an antidote for use-after-free.
I'm comparing this to munmap, which is actually such an antidote
(because it involves an IPI to flush all CPUs which could have seen the
mapping before).
I'm surprised that pkey_free doesn't perform a similar broadcast.
quoted
I think we should either implement revoke on pkey_alloc, with a
broadcast to all threads (the pkey_set race can be closed by having a
vDSO for that an the revocation code can check %rip to see if the old
PKRU value needs to be fixed up). Or we add the two pkey_alloc flags I
mentioned earlier.
That sounds awfully complicated to put in-kernel. I'd be happy to
review the patches after you put them together once we see how it looks.
TLB flushes are complicated, too, and very costly, but we still do them
on unmap, even in cases where they are not required for security reasons.
You basically want threads to broadcast their PKRU values at pkey_free()
time. That's totally doable... in userspace. You just need a mechanism
for each thread to periodically check if they need an update.
No, we want to the revocation to be immediate, so we'd have to use
something like the setxid broadcast, and we have to make sure that we
aren't in a pkey_set, and if we are, adjust register contents besides
PKRU. Not pretty at all. I really don't want to implement that.
If the broadcast is lazy, I think it defeats its purpose because you
don't know what kind of access privileges other threads in the system have.
Your solution to all MPK problems seems to be to say that it's undefined
and applications shouldn't do that. But if applications only used
well-defined memory accesses, why would we need MPK?
Thanks,
Florian
Using the malloc() analogy, we
don't expect that free() in one thread actively takes away references to
the memory held by other threads.
But malloc/free isn't expected to be a partial antidote to random
pointer scribbling.
Nor is protection keys intended to be an antidote for use-after-free.
I'm comparing this to munmap, which is actually such an antidote
(because it involves an IPI to flush all CPUs which could have seen the
mapping before).
I'm surprised that pkey_free doesn't perform a similar broadcast.
Hmm, I'm not sure this comparison is accurate. IPI flushes in unmap are
done because the shared page tables were updated, and TLB's in other
cpu's might be stale. The closest pkey equivalent would be allocating a
new pkey that only my thread can use, and then using it in
pkey_mprotect() to change some memory region. Then other threads will
lose access and I believe IPI's will be issued and existing TLB mappings
in other cpu's removed.
pkey_remove() has AFAICS two potential problems:
- the key is still used in some page tables. Scanning them all and
resetting to 0 would be rather expensive. Maybe we could maintain
per-pkey counters (for pkey != 0) in the mm, which might not be that
expensive, and refuse pkey_free() if the counter is not zero?
- the key is still "used" by other threads in their PKRU. Here I would
think that if kernel doesn't broadcast pkey_alloc() to other threads, it
also shouldn't broadcast the freeing? We also can't track per-pkey
"threads using pkey" counters, as WRPKRU is pure userspace.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-23 15:25:58
On 11/23/2017 04:48 AM, Florian Weimer wrote:
On 11/09/2017 05:59 PM, Dave Hansen wrote:
quoted
The manpage is pretty bare here. But the thought was that, in most
cases, you will want to allocate a key and start using it immediately.
This was in response to some feedback on one of the earlier reviews of
the patch set.
Okay. In the future, may want to use this access rights to specify the
default for the signal handler (with a new pkey_alloc flag). If I can
the default access rights, that would pretty much solve the sigsetjmp
problem for me, too, and I can start using protection keys in low-level
glibc code.
I haven't thought about this much in a year or so, but I think this is
doable.
One bit of advice: please look at features when they go in to the
kernel. Your feedback has been valuable, but not very timely. I
promise you'll get better results if you give feedback when patches are
being posted rather than when they've been in the kernel for a year.
quoted
quoted
I think we should either implement revoke on pkey_alloc, with a
broadcast to all threads (the pkey_set race can be closed by having a
vDSO for that an the revocation code can check %rip to see if the old
PKRU value needs to be fixed up). Or we add the two pkey_alloc flags I
mentioned earlier.
That sounds awfully complicated to put in-kernel. I'd be happy to
review the patches after you put them together once we see how it looks.
TLB flushes are complicated, too, and very costly, but we still do them
on unmap, even in cases where they are not required for security reasons.
I'll also note that TLB flushes are transparent to software. What you
are suggesting is not. That makes it a *LOT* more difficult to implement.
If you have an idea how to do this, I'll happily review patches!
quoted
You basically want threads to broadcast their PKRU values at pkey_free()
time. That's totally doable... in userspace. You just need a mechanism
for each thread to periodically check if they need an update.
No, we want to the revocation to be immediate, so we'd have to use
something like the setxid broadcast, and we have to make sure that we
aren't in a pkey_set, and if we are, adjust register contents besides
PKRU. Not pretty at all. I really don't want to implement that.
If the broadcast is lazy, I think it defeats its purpose because you
don't know what kind of access privileges other threads in the system have.
Your solution to all MPK problems seems to be to say that it's undefined
and applications shouldn't do that. But if applications only used
well-defined memory accesses, why would we need MPK?
BTW, I never call this feature MPK because it looks too much like MPX
and they have nothing to do with each other. I'd recommend the same to
you. It keeps your audience less confused.
I understand there is some distaste for where the implementation
settled. I don't, either, in a lot of ways. If I were to re-architect
it in the CPU, I certainly wouldn't have a user-visible PKRU and and
found a way to avoid the signal PKRU issues. But, that ship has sailed.
I don't see a way to do a broadcast PKRU update. But, I'd love to be
proven wrong, with code.
I don't see a way to do a broadcast PKRU update. But, I'd love to be
proven wrong, with code.
I could use the existing setxid broadcast code in glibc to update PKRU
on all running threads upon a key allocation (before pkey_alloc returns
to the application), but this won't work for the implicit protection key
used for PROT_EXEC. I don't see a good way to get its number, and to
determine whether a particular mprotect call allocated it. (We
obviously don't want to do the broadcast on every mprotect call with
PROT_EXEC, just in case.)
What's worse, the setxid broadcast is not async-signal-safe, so we can't
use it from mprotect, which should better be async-signal-safe (I know
that official, it's not, but it would still be problematic to change
that IMHO).
(The setxid broadcast mechanism allows us to run a piece of code on all
threads of the process. We could look at %rip and see if the signal
arrived during a pkey_set function call, and make sure that this call
delivers the right result, by altering the task state before returning.)
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
I'm working on adding memory protection key support to glibc.
I don't think pkey_free, as it is implemented today, is very safe due to
key reuse by a subsequent pkey_alloc. I see two problems:
(A) pkey_free allows reuse for they key while there are still mappings
that use it.
(B) If a key is reused, existing threads retain their access rights,
while there is an expectation that pkey_alloc denies access for the
threads except the current one.
I have a somewhat related question to API/documentation of pkeys, that
came up from a customer interested in using the feature. The man page of
mprotect/pkey_mprotect doesn't say how to remove a pkey from a set of
pages, i.e. reset it to the default 0 (or the exec-only pkey), so
initially they thought there's no way to do that.
Calling pkey_mprotect() with pkey==0 will fail with EINVAL, because 0
was not allocated by pkey_alloc(). That's fair I guess.
What seems to work to reset the pkey is either calling plain mprotect(),
or calling pkey_mprotect() with pkey == -1, as the former is just wired
to the latter.
So, is plain mprotect() the intended way to reset a pkey and should it
be explicitly documented in the man page?
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
Thanks,
Vlastimil
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
I plan to document this behavior on the glibc side, and glibc will call
mprotect (not pkey_mprotect) for key -1, so that you won't get ENOSYS
with kernels which do not support pkey_mprotect.
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
Hmm the current manpage says then when MPK is not supported, pkey has to
be specified 0. Which is a value that doesn't work when MPK *is*
supported. So -1 is more universal indeed.
I plan to document this behavior on the glibc side, and glibc will call
mprotect (not pkey_mprotect) for key -1, so that you won't get ENOSYS
with kernels which do not support pkey_mprotect.
Fair enough. What will you do about pkey_alloc() in that case, emulate
ENOSPC? Oh, the manpage already suggests so. And the return value in
that case is... -1. Makes sense :)
Thanks,
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
Hmm the current manpage says then when MPK is not supported, pkey has to
be specified 0. Which is a value that doesn't work when MPK *is*
supported. So -1 is more universal indeed.
-1 also chosen a different key if key 0 does not support the requested
protection flags.
quoted
I plan to document this behavior on the glibc side, and glibc will call
mprotect (not pkey_mprotect) for key -1, so that you won't get ENOSYS
with kernels which do not support pkey_mprotect.
Fair enough. What will you do about pkey_alloc() in that case, emulate
ENOSPC? Oh, the manpage already suggests so. And the return value in
that case is... -1. Makes sense :)
The manual page is incorrect, the kernel actually returns EINVAL.
Applications should check for EINVAL (and also ENOSYS) and activate
fallback code. Using -1 directly would be a bit reckless IMHO.
Thanks
Florian
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-22 16:10:16
On 11/22/2017 04:15 AM, Florian Weimer wrote:
On 11/22/2017 09:18 AM, Vlastimil Babka wrote:
quoted
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
On the other hand, x86-64 has no single default protection key due to
the PROT_EXEC emulation.
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-22 16:32:15
On 11/22/2017 08:21 AM, Florian Weimer wrote:
On 11/22/2017 05:10 PM, Dave Hansen wrote:
quoted
On 11/22/2017 04:15 AM, Florian Weimer wrote:
quoted
On 11/22/2017 09:18 AM, Vlastimil Babka wrote:
quoted
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
On the other hand, x86-64 has no single default protection key due to
the PROT_EXEC emulation.
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API. The fact that it's implemented
with pkeys should be pretty immaterial other than the fact that you
can't touch the high bits in PKRU.
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
On the other hand, x86-64 has no single default protection key due to
the PROT_EXEC emulation.
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API. The fact that it's implemented
with pkeys should be pretty immaterial other than the fact that you
can't touch the high bits in PKRU.
So, just to be sure, if we call pkey_mprotect() with 0, will it blindly
set 0, or the result of arch_override_mprotect_pkey() (thus equivalent
to call with -1) ? I assume the latter?
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-23 15:00:37
On 11/23/2017 12:11 AM, Vlastimil Babka wrote:
quoted
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API. The fact that it's implemented
with pkeys should be pretty immaterial other than the fact that you
can't touch the high bits in PKRU.
So, just to be sure, if we call pkey_mprotect() with 0, will it blindly
set 0, or the result of arch_override_mprotect_pkey() (thus equivalent
to call with -1) ? I assume the latter?
It's supposed to set 0.
-1 was, as far as I remember, an internal-to-the-kernel-only thing to
tell us that a key came from *mprotect()* instead of pkey_mprotect().
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API. The fact that it's implemented
with pkeys should be pretty immaterial other than the fact that you
can't touch the high bits in PKRU.
So, just to be sure, if we call pkey_mprotect() with 0, will it blindly
set 0, or the result of arch_override_mprotect_pkey() (thus equivalent
to call with -1) ? I assume the latter?
It's supposed to set 0.
-1 was, as far as I remember, an internal-to-the-kernel-only thing to
tell us that a key came from *mprotect()* instead of pkey_mprotect().
So, pkey_mprotect(..., 0) will set it to 0, regardless of PROT_EXEC.
pkey_mprotect(..., -1) or mprotect() will set it to 0-or-PROT_EXEC-pkey.
Can't shake the feeling that it's somewhat weird, but I guess it's
flexible at least. So just has to be well documented.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-23 23:29:23
On 11/23/2017 01:42 PM, Vlastimil Babka wrote:
quoted
It's supposed to set 0.
-1 was, as far as I remember, an internal-to-the-kernel-only thing to
tell us that a key came from *mprotect()* instead of pkey_mprotect().
So, pkey_mprotect(..., 0) will set it to 0, regardless of PROT_EXEC.
Although weird, the thought here was that pkey_mprotect() callers are
new and should know about the interactions with PROT_EXEC. They can
also *get* PROT_EXEC semantics if they want.
The only wart here is if you do:
mprotect(..., PROT_EXEC); // key 10 is now the PROT_EXEC key
pkey_mprotect(..., PROT_EXEC, key=3);
I'm not sure what this does. We should probably ensure that it returns
an error.
pkey_mprotect(..., -1) or mprotect() will set it to 0-or-PROT_EXEC-pkey.
Can't shake the feeling that it's somewhat weird, but I guess it's
flexible at least. So just has to be well documented.
It *is* weird. But, layering on top of legacy APIs are often weird. I
would have been open to other sane, but less weird ways to do it a year
ago. :)
Although weird, the thought here was that pkey_mprotect() callers are
new and should know about the interactions with PROT_EXEC. They can
also*get* PROT_EXEC semantics if they want.
The only wart here is if you do:
mprotect(..., PROT_EXEC); // key 10 is now the PROT_EXEC key
I thought the PROT_EXEC key is always 1?
pkey_mprotect(..., PROT_EXEC, key=3);
I'm not sure what this does. We should probably ensure that it returns
an error.
Without protection key support, PROT_EXEC would imply PROT_READ with an
ordinary mprotect. I think it makes sense to stick to this behavior.
It is what I have documented for glibc:
<https://sourceware.org/ml/libc-alpha/2017-11/msg00841.html>
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
Although weird, the thought here was that pkey_mprotect() callers are
new and should know about the interactions with PROT_EXEC. They can
also*get* PROT_EXEC semantics if they want.
The only wart here is if you do:
mprotect(..., PROT_EXEC); // key 10 is now the PROT_EXEC key
I thought the PROT_EXEC key is always 1?
Seems it assigns the first non-allocated one. Can even fail if there's
none left, and then there's no PROT_EXEC read protection. In practice I
expect PROT_EXEC mapping to be created by ELF loader (?) before the
program can even call pkey_alloc() itself, so it would be 1.
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”, and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
On the other hand, x86-64 has no single default protection key due to
the PROT_EXEC emulation.
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API.
I see key 1 in /proc for a PROT_EXEC mapping. If I supply an explicit
protection key, that key is used, and the page ends up having read
access enabled.
The key is also visible in the siginfo_t argument on read access to a
PROT_EXEC mapping with the default key, so it's not just /proc:
page 1 (0x7f008242d000): read access denied
SIGSEGV address: 0x7f008242d000
SIGSEGV code: 4
SIGSEGV key: 1
I'm attaching my test.
> The fact that it's implemented
> with pkeys should be pretty immaterial other than the fact that you
> can't touch the high bits in PKRU.
I don't see a restriction for PKRU updates. If I write zero to the PKRU
register, PROT_EXEC implies PROT_READ, as I would expect.
This is with kernel 4.14.
Florian
From: Dave Hansen <dave.hansen@linux.intel.com> Date: 2017-11-23 15:09:18
On 11/23/2017 04:38 AM, Florian Weimer wrote:
On 11/22/2017 05:32 PM, Dave Hansen wrote:
quoted
On 11/22/2017 08:21 AM, Florian Weimer wrote:
quoted
On 11/22/2017 05:10 PM, Dave Hansen wrote:
quoted
On 11/22/2017 04:15 AM, Florian Weimer wrote:
quoted
On 11/22/2017 09:18 AM, Vlastimil Babka wrote:
quoted
And, was the pkey == -1 internal wiring supposed to be exposed to the
pkey_mprotect() signal, or should there have been a pre-check
returning
EINVAL in SYSCALL_DEFINE4(pkey_mprotect), before calling
do_mprotect_pkey())? I assume it's too late to change it now
anyway (or
not?), so should we also document it?
I think the -1 case to the set the default key is useful because it
allows you to use a key value of -1 to mean “MPK is not supported”,
and
still call pkey_mprotect.
The behavior to not allow 0 to be set was unintentional and is a bug.
We should fix that.
On the other hand, x86-64 has no single default protection key due to
the PROT_EXEC emulation.
No, the default is clearly 0 and documented to be so. The PROT_EXEC
emulation one should be inaccessible in all the APIs so does not even
show up as *being* a key in the API.
I should have been more explicit: the EXEC pkey does not show up in the
syscall API.
I see key 1 in /proc for a PROT_EXEC mapping. If I supply an explicit
protection key, that key is used, and the page ends up having read
access enabled.
The key is also visible in the siginfo_t argument on read access to a
PROT_EXEC mapping with the default key, so it's not just /proc:
page 1 (0x7f008242d000): read access denied
SIGSEGV address: 0x7f008242d000
SIGSEGV code: 4
SIGSEGV key: 1
I'm attaching my test.
Yes, it is exposed there. But, as a non-allocated pkey, the intention
in the kernel was to make sure that it could not be passed to the syscalls.
If that behavior is broken, we should probably fix it.
quoted
The fact that it's implemented
with pkeys should be pretty immaterial other than the fact that you
can't touch the high bits in PKRU.
I don't see a restriction for PKRU updates. If I write zero to the PKRU
register, PROT_EXEC implies PROT_READ, as I would expect.
I'll rephrase:
The fact that it's implemented with pkeys should be pretty
immaterial other than the fact that you must not touch the bits
controlling PROT_EXEC in PKRU if you want to keep it working.
There is no restriction which is *enforced*. It's just documented.