| Message ID | 20180502132751.05B9F401F3041@oldenburg.str.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/02/2018 06:26 AM, Florian Weimer wrote: > pkeys support for IBM POWER intends to inherited the access rights of > the current thread in signal handlers. The advantage is that this > preserves access to memory regions associated with non-default keys, > enabling additional usage scenarios for memory protection keys which > currently do not work on x86 due to the unconditional reset to the > (configurable) default key in signal handlers. What's the usage scenario that does not work? > Consequently, this commit updates the x86 implementation to preserve > the PKRU register value of the interrupted context in signal handlers. > If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT > flag, the application can assume this signal inheritance behavior. I think this is a pretty gross misuse of the API. Adding an argument to pkey_alloc() is something that folks would assume would impact the key being *allocated*, not pkeys behavior across the process as a whole. > This change does not affect the init_pkru optimization because if the > thread's PKRU register is zero due to the init_pkru setting, it will > remain zero in the signal handler through inheritance from the > interrupted context. I think you are right, but it's rather convoluted. It does: 1. Running with PKRU in the init state 2. Kernel saves off init-state-PKRU XSAVE signal buffer 3. Enter signal, kernel XRSTOR (may) set the init state again 4. fpu__clear() does __write_pkru(), takes it out of the init state 5. Signal handler runs, exits 6. fpu__restore_sig() XRSTOR's the state from #2, taking PKRU back to the init state But, about the patch in general: I'm not a big fan of doing this in such a PKRU-specific way. It would be nice to have this available for all XSAVE states. It would also keep you from so unnecessarily frobbing with WRPKRU in fpu__clear(). You could just clear the PKRU bit in the Requested Feature BitMap (RFBM) passed to XRSTOR. That would be much straightforward and able to be more easily extended to more states. PKRU is now preserved on signal entry, but not signal exit. Was that intentional? That seems like odd behavior, and also differs from the POWER implementation as I understand it.
On 05/02/2018 04:30 PM, Dave Hansen wrote: > On 05/02/2018 06:26 AM, Florian Weimer wrote: >> pkeys support for IBM POWER intends to inherited the access rights of >> the current thread in signal handlers. The advantage is that this >> preserves access to memory regions associated with non-default keys, >> enabling additional usage scenarios for memory protection keys which >> currently do not work on x86 due to the unconditional reset to the >> (configurable) default key in signal handlers. > > What's the usage scenario that does not work? Here's what I want to do: Nick Clifton wrote a binutils patch which puts the .got.plt section on separate pages. We allocate a protection key for it, assign it to all such sections in the process image, and change the access rights of the main thread to disallow writes via that key during process startup. In _dl_fixup, we enable write access to the GOT, update the GOT entry, and then disable it again. This way, we have a pretty safe form of lazy binding, without having to resort to BIND_NOW. With the current kernel behavior on x86, we cannot do that because signal handlers revert to the default (deny) access rights, so the GOT turns inaccessible. >> Consequently, this commit updates the x86 implementation to preserve >> the PKRU register value of the interrupted context in signal handlers. >> If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT >> flag, the application can assume this signal inheritance behavior. > > I think this is a pretty gross misuse of the API. Adding an argument to > pkey_alloc() is something that folks would assume would impact the key > being *allocated*, not pkeys behavior across the process as a whole. From the application point of view, only the allocated key is affected—it has specific semantics that were undefined before and varied between x86 and POWER. >> This change does not affect the init_pkru optimization because if the >> thread's PKRU register is zero due to the init_pkru setting, it will >> remain zero in the signal handler through inheritance from the >> interrupted context. > > I think you are right, but it's rather convoluted. It does: > > 1. Running with PKRU in the init state > 2. Kernel saves off init-state-PKRU XSAVE signal buffer > 3. Enter signal, kernel XRSTOR (may) set the init state again > 4. fpu__clear() does __write_pkru(), takes it out of the init state > 5. Signal handler runs, exits > 6. fpu__restore_sig() XRSTOR's the state from #2, taking PKRU back to > the init state Isn't that just the cost of not hard-coding the XSAVE area layout? > But, about the patch in general: > > I'm not a big fan of doing this in such a PKRU-specific way. It would > be nice to have this available for all XSAVE states. It would also keep > you from so unnecessarily frobbing with WRPKRU in fpu__clear(). You > could just clear the PKRU bit in the Requested Feature BitMap (RFBM) > passed to XRSTOR. That would be much straightforward and able to be > more easily extended to more states. I don't see where I could plug this into the current kernel sources. Would you please provide some pointers? > PKRU is now preserved on signal entry, but not signal exit. Was that > intentional? That seems like odd behavior, and also differs from the > POWER implementation as I understand it. Ram, would you please comment? I think it is a bug not restore the access rights to the former value in the interrupted context. In userspace, we have exactly this problem with errno, and it can lead to subtle bugs. Thanks, Florian
On 05/02/2018 08:12 AM, Florian Weimer wrote: > On 05/02/2018 04:30 PM, Dave Hansen wrote: >> On 05/02/2018 06:26 AM, Florian Weimer wrote: >>> pkeys support for IBM POWER intends to inherited the access rights of >>> the current thread in signal handlers. The advantage is that this >>> preserves access to memory regions associated with non-default keys, >>> enabling additional usage scenarios for memory protection keys which >>> currently do not work on x86 due to the unconditional reset to the >>> (configurable) default key in signal handlers. >> >> What's the usage scenario that does not work? > > Here's what I want to do: > > Nick Clifton wrote a binutils patch which puts the .got.plt section on > separate pages. We allocate a protection key for it, assign it to all > such sections in the process image, and change the access rights of the > main thread to disallow writes via that key during process startup. In > _dl_fixup, we enable write access to the GOT, update the GOT entry, and > then disable it again. > > This way, we have a pretty safe form of lazy binding, without having to > resort to BIND_NOW. > > With the current kernel behavior on x86, we cannot do that because > signal handlers revert to the default (deny) access rights, so the GOT > turns inaccessible. cc'ing Andy Lutomirksi... He was the one that specifically asked for the deny-by-default behavior that I think is biting you. The behavior we had before implementing Andy's suggestion was, I think, the one you would want: we had allow-all as the default PKRU value. The other option here that I think we discussed in the past was to have an *explicit* signal PKRU value. That way, we can be restrictive by default but allow overrides for special cases like you have. >>> Consequently, this commit updates the x86 implementation to preserve >>> the PKRU register value of the interrupted context in signal handlers. >>> If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT >>> flag, the application can assume this signal inheritance behavior. >> >> I think this is a pretty gross misuse of the API. Adding an argument to >> pkey_alloc() is something that folks would assume would impact the key >> being *allocated*, not pkeys behavior across the process as a whole. > > From the application point of view, only the allocated key is > affected—it has specific semantics that were undefined before and varied > between x86 and POWER. I'd really like to see it in a separate API. >>> This change does not affect the init_pkru optimization because if the >>> thread's PKRU register is zero due to the init_pkru setting, it will >>> remain zero in the signal handler through inheritance from the >>> interrupted context. >> >> I think you are right, but it's rather convoluted. It does: >> >> 1. Running with PKRU in the init state >> 2. Kernel saves off init-state-PKRU XSAVE signal buffer >> 3. Enter signal, kernel XRSTOR (may) set the init state again >> 4. fpu__clear() does __write_pkru(), takes it out of the init state >> 5. Signal handler runs, exits >> 6. fpu__restore_sig() XRSTOR's the state from #2, taking PKRU back to >> the init state > > Isn't that just the cost of not hard-coding the XSAVE area layout? No. You unnecessarily take PKRU out of the init state. If you don't want to change its value, just don't do that when you run XRSTOR. >> But, about the patch in general: >> >> I'm not a big fan of doing this in such a PKRU-specific way. It would >> be nice to have this available for all XSAVE states. It would also keep >> you from so unnecessarily frobbing with WRPKRU in fpu__clear(). You >> could just clear the PKRU bit in the Requested Feature BitMap (RFBM) >> passed to XRSTOR. That would be much straightforward and able to be >> more easily extended to more states. > > I don't see where I could plug this into the current kernel sources. > Would you please provide some pointers? In handle_signal()->fpu__clear()->...__copy_kernel_to_fpregs(), we pass -1 as the RFBM. Don't do that. Pass (~0 & ~XFEATURE_MASK_PKRU).
> On May 2, 2018, at 8:12 AM, Florian Weimer <fweimer@redhat.com> wrote: > >> On 05/02/2018 04:30 PM, Dave Hansen wrote: >>> On 05/02/2018 06:26 AM, Florian Weimer wrote: >>> pkeys support for IBM POWER intends to inherited the access rights of >>> the current thread in signal handlers. The advantage is that this >>> preserves access to memory regions associated with non-default keys, >>> enabling additional usage scenarios for memory protection keys which >>> currently do not work on x86 due to the unconditional reset to the >>> (configurable) default key in signal handlers. >> What's the usage scenario that does not work? > > Here's what I want to do: > > Nick Clifton wrote a binutils patch which puts the .got.plt section on separate pages. We allocate a protection key for it, assign it to all such sections in the process image, and change the access rights of the main thread to disallow writes via that key during process startup. In _dl_fixup, we enable write access to the GOT, update the GOT entry, and then disable it again. > > This way, we have a pretty safe form of lazy binding, without having to resort to BIND_NOW. > > With the current kernel behavior on x86, we cannot do that because signal handlers revert to the default (deny) access rights, so the GOT turns inaccessible. Dave is right: the current behavior was my request, and I still think it’s correct. The whole point is that, even if something nasty happens like a SIGALRM handler hitting in the middle of _dl_fixup, the SIGALRM handler is preventing from accidentally writing to the protected memory. When SIGALRM returns, PKRU should get restored Another way of looking at this is that the kernel would like to approximate what the ISA behavior *should* have been: the whole sequence “modify PKRU; access memory; restore PKRU” should be as atomic as possible. Florian, what is the actual problematic sequence of events? —Andy
On 05/02/2018 07:09 PM, Andy Lutomirski wrote: >> Nick Clifton wrote a binutils patch which puts the .got.plt section on separate pages. We allocate a protection key for it, assign it to all such sections in the process image, and change the access rights of the main thread to disallow writes via that key during process startup. In _dl_fixup, we enable write access to the GOT, update the GOT entry, and then disable it again. >> >> This way, we have a pretty safe form of lazy binding, without having to resort to BIND_NOW. >> >> With the current kernel behavior on x86, we cannot do that because signal handlers revert to the default (deny) access rights, so the GOT turns inaccessible. > Dave is right: the current behavior was my request, and I still think it’s correct. The whole point is that, even if something nasty happens like a SIGALRM handler hitting in the middle of _dl_fixup, the SIGALRM handler is preventing from accidentally writing to the protected memory. When SIGALRM returns, PKRU should get restored > > Another way of looking at this is that the kernel would like to approximate what the ISA behavior*should* have been: the whole sequence “modify PKRU; access memory; restore PKRU” should be as atomic as possible. > > Florian, what is the actual problematic sequence of events? See above. The signal handler will crash if it calls any non-local function through the GOT because with the default access rights, it's not readable in the signal handler. Any use of memory protection keys for basic infrastructure will run into this problem, so I think the current kernel behavior is not very useful. It's also x86-specific. From a security perspective, the atomic behavior is not very useful because you generally want to modify PKRU *before* computing the details of the memory access, so that you don't have a general “poke anywhere with this access right” primitive in the text segment. (I called this the “suffix problem” in another context.) For this reason, I plan to add the PKRU modification to the beginning of _dl_fixup. CET will offer a different trade-off here, but we haven't that yet. Thanks, Florian
On Wed, May 2, 2018 at 10:17 AM Florian Weimer <fweimer@redhat.com> wrote: > On 05/02/2018 07:09 PM, Andy Lutomirski wrote: > >> Nick Clifton wrote a binutils patch which puts the .got.plt section on separate pages. We allocate a protection key for it, assign it to all such sections in the process image, and change the access rights of the main thread to disallow writes via that key during process startup. In _dl_fixup, we enable write access to the GOT, update the GOT entry, and then disable it again. > >> > >> This way, we have a pretty safe form of lazy binding, without having to resort to BIND_NOW. > >> > >> With the current kernel behavior on x86, we cannot do that because signal handlers revert to the default (deny) access rights, so the GOT turns inaccessible. > > Dave is right: the current behavior was my request, and I still think it’s correct. The whole point is that, even if something nasty happens like a SIGALRM handler hitting in the middle of _dl_fixup, the SIGALRM handler is preventing from accidentally writing to the protected memory. When SIGALRM returns, PKRU should get restored > > > > Another way of looking at this is that the kernel would like to approximate what the ISA behavior*should* have been: the whole sequence “modify PKRU; access memory; restore PKRU” should be as atomic as possible. > > > > Florian, what is the actual problematic sequence of events? > See above. The signal handler will crash if it calls any non-local > function through the GOT because with the default access rights, it's > not readable in the signal handler. > Any use of memory protection keys for basic infrastructure will run into > this problem, so I think the current kernel behavior is not very useful. > It's also x86-specific. > From a security perspective, the atomic behavior is not very useful > because you generally want to modify PKRU *before* computing the details > of the memory access, so that you don't have a general “poke anywhere > with this access right” primitive in the text segment. (I called this > the “suffix problem” in another context.) Ugh, right. It's been long enough that I forgot about the underlying issue. A big part of the problem here is that pkey_alloc() should set the initial value of the key across all threads, but it *can't*. There is literally no way to do it in a multithreaded program that uses RDPKRU and WRPKRU. But I think the right fix, at least for your use case, is to have a per-mm init_pkru variable that starts as "deny all". We'd add a new pkey_alloc() flag like PKEY_ALLOC_UPDATE_INITIAL_STATE that causes the specified mode to update init_pkru. New threads and delivered signals would get the init_pkru state instead of the hardcoded default.
On 05/02/2018 10:41 PM, Andy Lutomirski wrote: >> See above. The signal handler will crash if it calls any non-local >> function through the GOT because with the default access rights, it's >> not readable in the signal handler. > >> Any use of memory protection keys for basic infrastructure will run into >> this problem, so I think the current kernel behavior is not very useful. >> It's also x86-specific. > >> From a security perspective, the atomic behavior is not very useful >> because you generally want to modify PKRU *before* computing the details >> of the memory access, so that you don't have a general “poke anywhere >> with this access right” primitive in the text segment. (I called this >> the “suffix problem” in another context.) > > > Ugh, right. It's been long enough that I forgot about the underlying > issue. A big part of the problem here is that pkey_alloc() should set the > initial value of the key across all threads, but it *can't*. There is > literally no way to do it in a multithreaded program that uses RDPKRU and > WRPKRU. The kernel could do *something*, probably along the membarrier system call. I mean, I could implement a reasonable close approximation in userspace, via the setxid mechanism in glibc (but I really don't want to). > But I think the right fix, at least for your use case, is to have a per-mm > init_pkru variable that starts as "deny all". We'd add a new pkey_alloc() > flag like PKEY_ALLOC_UPDATE_INITIAL_STATE that causes the specified mode to > update init_pkru. New threads and delivered signals would get the > init_pkru state instead of the hardcoded default. I implemented this for signal handlers: https://marc.info/?l=linux-api&m=151285420302698&w=2 This does not alter the thread inheritance behavior yet. I would have to investigate how to implement that. Feedback led to the current patch, though. I'm not sure what has changed since then. If I recall correctly, the POWER maintainer did express a strong desire back then for (what is, I believe) their current semantics, which my PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. Thanks, Florian
On 05/02/2018 05:28 PM, Dave Hansen wrote: > The other option here that I think we discussed in the past was to have > an*explicit* signal PKRU value. That way, we can be restrictive by > default but allow overrides for special cases like you have. That's the patch I posted before (with PKEY_ALLOC_SETSIGNAL). I'm afraid we are going in circles. Thanks, Florian
On Wed, May 02, 2018 at 05:12:50PM +0200, Florian Weimer wrote: > On 05/02/2018 04:30 PM, Dave Hansen wrote: > >On 05/02/2018 06:26 AM, Florian Weimer wrote: > >>pkeys support for IBM POWER intends to inherited the access rights of > >>the current thread in signal handlers. The advantage is that this > >>preserves access to memory regions associated with non-default keys, > >>enabling additional usage scenarios for memory protection keys which > >>currently do not work on x86 due to the unconditional reset to the > >>(configurable) default key in signal handlers. > > > >What's the usage scenario that does not work? > > Here's what I want to do: > > Nick Clifton wrote a binutils patch which puts the .got.plt section > on separate pages. We allocate a protection key for it, assign it > to all such sections in the process image, and change the access > rights of the main thread to disallow writes via that key during > process startup. In _dl_fixup, we enable write access to the GOT, > update the GOT entry, and then disable it again. > > This way, we have a pretty safe form of lazy binding, without having > to resort to BIND_NOW. > > With the current kernel behavior on x86, we cannot do that because > signal handlers revert to the default (deny) access rights, so the > GOT turns inaccessible. > > >>Consequently, this commit updates the x86 implementation to preserve > >>the PKRU register value of the interrupted context in signal handlers. > >>If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT > >>flag, the application can assume this signal inheritance behavior. > > > >I think this is a pretty gross misuse of the API. Adding an argument to > >pkey_alloc() is something that folks would assume would impact the key > >being *allocated*, not pkeys behavior across the process as a whole. > > From the application point of view, only the allocated key is > affected—it has specific semantics that were undefined before and > varied between x86 and POWER. > > >>This change does not affect the init_pkru optimization because if the > >>thread's PKRU register is zero due to the init_pkru setting, it will > >>remain zero in the signal handler through inheritance from the > >>interrupted context. > > > >I think you are right, but it's rather convoluted. It does: > > > >1. Running with PKRU in the init state > >2. Kernel saves off init-state-PKRU XSAVE signal buffer > >3. Enter signal, kernel XRSTOR (may) set the init state again > >4. fpu__clear() does __write_pkru(), takes it out of the init state > >5. Signal handler runs, exits > >6. fpu__restore_sig() XRSTOR's the state from #2, taking PKRU back to > > the init state > > Isn't that just the cost of not hard-coding the XSAVE area layout? > > >But, about the patch in general: > > > >I'm not a big fan of doing this in such a PKRU-specific way. It would > >be nice to have this available for all XSAVE states. It would also keep > >you from so unnecessarily frobbing with WRPKRU in fpu__clear(). You > >could just clear the PKRU bit in the Requested Feature BitMap (RFBM) > >passed to XRSTOR. That would be much straightforward and able to be > >more easily extended to more states. > > I don't see where I could plug this into the current kernel sources. > Would you please provide some pointers? > > >PKRU is now preserved on signal entry, but not signal exit. Was that > >intentional? That seems like odd behavior, and also differs from the > >POWER implementation as I understand it. > > Ram, would you please comment? on POWER the pkey behavior will remain the same at entry or at exit from the signal handler. For eg: if a key is read-disabled on entry into the signal handler, and gets read-enabled in the signal handler, than it will continue to be read-enabled on return from the signal handler. In other words, changes to key permissions persist across signal boundaries. > > I think it is a bug not restore the access rights to the former > value in the interrupted context. In userspace, we have exactly > this problem with errno, and it can lead to subtle bugs. > > Thanks, > Florian
On Wed, May 2, 2018 at 2:13 PM Ram Pai <linuxram@us.ibm.com> wrote: > > Ram, would you please comment? > on POWER the pkey behavior will remain the same at entry or at exit from > the signal handler. For eg: if a key is read-disabled on entry into > the signal handler, and gets read-enabled in the signal handler, than it > will continue to be read-enabled on return from the signal handler. > In other words, changes to key permissions persist across signal > boundaries. I don't know about POWER's ISA, but this is crappy behavior. If a thread temporarily grants itself access to a restrictive memory key and then gets a signal, the signal handler should *not* have access to that key.
On Wed, May 2, 2018 at 2:06 PM Florian Weimer <fweimer@redhat.com> wrote: > On 05/02/2018 10:41 PM, Andy Lutomirski wrote: > >> See above. The signal handler will crash if it calls any non-local > >> function through the GOT because with the default access rights, it's > >> not readable in the signal handler. > > > >> Any use of memory protection keys for basic infrastructure will run into > >> this problem, so I think the current kernel behavior is not very useful. > >> It's also x86-specific. > > > >> From a security perspective, the atomic behavior is not very useful > >> because you generally want to modify PKRU *before* computing the details > >> of the memory access, so that you don't have a general “poke anywhere > >> with this access right” primitive in the text segment. (I called this > >> the “suffix problem” in another context.) > > > > > > Ugh, right. It's been long enough that I forgot about the underlying > > issue. A big part of the problem here is that pkey_alloc() should set the > > initial value of the key across all threads, but it *can't*. There is > > literally no way to do it in a multithreaded program that uses RDPKRU and > > WRPKRU. > The kernel could do *something*, probably along the membarrier system > call. I mean, I could implement a reasonable close approximation in > userspace, via the setxid mechanism in glibc (but I really don't want to). I beg to differ. Thread A: old = RDPKRU(); WRPKRU(old & ~3); ... WRPKRU(old); Thread B: pkey_alloc(). If pkey_alloc() happens while thread A is in the ... part, you lose. It makes no difference what the kernel does. The problem is that the WRPKRU instruction itself is designed incorrectly. > > But I think the right fix, at least for your use case, is to have a per-mm > > init_pkru variable that starts as "deny all". We'd add a new pkey_alloc() > > flag like PKEY_ALLOC_UPDATE_INITIAL_STATE that causes the specified mode to > > update init_pkru. New threads and delivered signals would get the > > init_pkru state instead of the hardcoded default. > I implemented this for signal handlers: > https://marc.info/?l=linux-api&m=151285420302698&w=2 > This does not alter the thread inheritance behavior yet. I would have > to investigate how to implement that. > Feedback led to the current patch, though. I'm not sure what has > changed since then. What feedback? I think the old patch was much better than the new patch. I could point out some issues in the kernel code, and I think it should deal with thread creation, but otherwise I think it's the right approach. Keep in mind, though, that it's just not possible to make pkey_alloc() work on x86 in any sensible way in a multithreaded program. > If I recall correctly, the POWER maintainer did express a strong desire > back then for (what is, I believe) their current semantics, which my > PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. Ram, I really really don't like the POWER semantics. Can you give some justification for them? Does POWER at least have an atomic way for userspace to modify just the key it wants to modify or, even better, special load and store instructions to use alternate keys? --Andy
On 05/02/2018 02:08 PM, Florian Weimer wrote: > On 05/02/2018 05:28 PM, Dave Hansen wrote: >> The other option here that I think we discussed in the past was to have >> an*explicit* signal PKRU value. That way, we can be restrictive by >> default but allow overrides for special cases like you have. > > That's the patch I posted before (with PKEY_ALLOC_SETSIGNAL). I'm > afraid we are going in circles. Could you remind us why you abandoned that approach and its relative merits versus this new approach?
On 05/02/2018 02:23 PM, Andy Lutomirski wrote: >> The kernel could do *something*, probably along the membarrier system >> call. I mean, I could implement a reasonable close approximation in >> userspace, via the setxid mechanism in glibc (but I really don't want to). > > I beg to differ. > > Thread A: > old = RDPKRU(); > WRPKRU(old & ~3); > ... > WRPKRU(old); > > Thread B: > pkey_alloc(). > > If pkey_alloc() happens while thread A is in the ... part, you lose. It > makes no difference what the kernel does. The problem is that the WRPKRU > instruction itself is designed incorrectly. Yes, *if* we define pkey_alloc() to be implicitly changing other threads' PKRU value. Let's say we go to the hardware guys and ask for a new instruction to fix this. We're going to have to make a pretty good case that this is either impossible or really hard to do in software. Surely we have the locking to tell another thread that we want its PKRU value to change without actively going out and having the kernel poke a new value in.
On Wed, May 2, 2018 at 3:08 PM Dave Hansen <dave.hansen@intel.com> wrote: > On 05/02/2018 02:23 PM, Andy Lutomirski wrote: > >> The kernel could do *something*, probably along the membarrier system > >> call. I mean, I could implement a reasonable close approximation in > >> userspace, via the setxid mechanism in glibc (but I really don't want to). > > > > I beg to differ. > > > > Thread A: > > old = RDPKRU(); > > WRPKRU(old & ~3); > > ... > > WRPKRU(old); > > > > Thread B: > > pkey_alloc(). > > > > If pkey_alloc() happens while thread A is in the ... part, you lose. It > > makes no difference what the kernel does. The problem is that the WRPKRU > > instruction itself is designed incorrectly. > Yes, *if* we define pkey_alloc() to be implicitly changing other > threads' PKRU value. I think that's the only generally useful behavior. In general, if libraries are going to use protection keys, they're going to do this: int key = pkey_alloc(...); mmap() or mprotect_key() some memory (for crypto secrets, a database, whatever). ... enable_write_access(key); write to the memory; disable_write_access(key); That library wants other threads, signal handlers, and, in general, the whole rest of the process to be restricted, and that library doesn't want race conditions. The problem here is that, to get this right, we either need the PKRU modifications to be syscalls or to take locks, and the lock approach is going to be fairly gross. > Let's say we go to the hardware guys and ask for a new instruction to > fix this. We're going to have to make a pretty good case that this is > either impossible or really hard to do in software. > Surely we have the locking to tell another thread that we want its PKRU > value to change without actively going out and having the kernel poke a > new value in. I think that doing it in userspace without a new instruction is going to be slow, ugly, unreliable, or more than one of the above. Here's my proposal. We ask the hardware folks for new instructions. Once we get tentative agreement, we add new vDSO pkey accessors. At first those accessors function will do a syscall. When we get the new instructions, they get alternatives. The vDSO helpers could look like this: typedef struct { u64 opaque; } pkey_save_t; pkey_save_t pkey_save_and_set(int key, unsigned int mode); void pkey_set(int key, unsigned int mode); void pkey_restore(int key, pkey_save_t prev); Slight variants are possible. On new hardware, pkey_set() and pkey_restore() use the WRPKRU replacement. pkey_save_and_set() uses the RDPKRU replacement followed by the WRPKRU replacement. The usage is: pkey_save_t prev = pkey_save_and_set(my_key, 0); /* enable read and write */ do something with memory; pkey_restore(my_key, prev); or just: pkey_set(my_key, 0); ... pkey_set(my_key, 3); /* or 1 or 2 depending on the application */ And we make it very, very clear to user code that using RDPKRU and WRPKRU directly is a big no-no. What do you all think? This gives us sane, if slow, behavior for current CPUs and is decently fast on hypothetical new CPUs.
On 05/02/2018 03:22 PM, Andy Lutomirski wrote: > That library wants other threads, signal handlers, and, in general, the > whole rest of the process to be restricted, and that library doesn't want > race conditions. The problem here is that, to get this right, we either > need the PKRU modifications to be syscalls or to take locks, and the lock > approach is going to be fairly gross. I totally get the idea that a RDPKRU/WRPKRU is non-atomic and that it can't be mixed with asynchronous WRPKRU's in that thread. But, where do those come from in this scenario? I'm not getting the secondary mechanism is that *makes* them unsafe.
> On May 2, 2018, at 3:32 PM, Dave Hansen <dave.hansen@intel.com> wrote: > >> On 05/02/2018 03:22 PM, Andy Lutomirski wrote: >> That library wants other threads, signal handlers, and, in general, the >> whole rest of the process to be restricted, and that library doesn't want >> race conditions. The problem here is that, to get this right, we either >> need the PKRU modifications to be syscalls or to take locks, and the lock >> approach is going to be fairly gross. > > I totally get the idea that a RDPKRU/WRPKRU is non-atomic and that it > can't be mixed with asynchronous WRPKRU's in that thread. > > But, where do those come from in this scenario? I'm not getting the > secondary mechanism is that *makes* them unsafe. pkey_alloc() itself. If someone tries to allocate a key with a given default mode, unless there’s already a key that already had that value in all threads or pkey_alloc() needs to asynchronously create such a key. There is a partial hack that glibc could do. DSOs could have a way to statically request a key (e.g. a PT_PKEY segment) and glibc could do all the pkey_alloc() calls before any threads get created. Of course, a DSO like this can’t be dlopened(). We still need a way for pkey_alloc() to update the value for signal delivery, but that’s straightforward.
On Wed, May 02, 2018 at 09:18:11PM +0000, Andy Lutomirski wrote: > On Wed, May 2, 2018 at 2:13 PM Ram Pai <linuxram@us.ibm.com> wrote: > > > > > Ram, would you please comment? > > > on POWER the pkey behavior will remain the same at entry or at exit from > > the signal handler. For eg: if a key is read-disabled on entry into > > the signal handler, and gets read-enabled in the signal handler, than it > > will continue to be read-enabled on return from the signal handler. > > > In other words, changes to key permissions persist across signal > > boundaries. > > I don't know about POWER's ISA, but this is crappy behavior. If a thread > temporarily grants itself access to a restrictive memory key and then gets > a signal, the signal handler should *not* have access to that key. This is a new requirement that I was not aware off. Its not documented anywhere AFAICT. Regardless of how the ISA behaves, its still a kernel behavior that needs to be clearly defined.
On 05/02/2018 04:32 PM, Andy Lutomirski wrote: >> But, where do those come from in this scenario? I'm not getting >> the secondary mechanism is that *makes* them unsafe. > pkey_alloc() itself. If someone tries to allocate a key with a given > default mode, unless there’s already a key that already had that > value in all threads or pkey_alloc() needs to asynchronously create > such a key. I think you are saying: If a thread calls pkey_alloc(), all threads should, by default, implicitly get access. That broadcast-to-other-threads is the thing that the current architecture doesn't do. In this situation, CPU threads have to go opt-out of getting access to data protected with a given, allocated key. Right?
> On May 2, 2018, at 4:58 PM, Dave Hansen <dave.hansen@intel.com> wrote: > > On 05/02/2018 04:32 PM, Andy Lutomirski wrote: >>> But, where do those come from in this scenario? I'm not getting >>> the secondary mechanism is that *makes* them unsafe. >> pkey_alloc() itself. If someone tries to allocate a key with a given >> default mode, unless there’s already a key that already had that >> value in all threads or pkey_alloc() needs to asynchronously create >> such a key. > > I think you are saying: If a thread calls pkey_alloc(), all threads > should, by default, implicitly get access. No, I’m saying that all threads should get the *requested* access. If I’m protecting the GOT, I want all threads to get RO access. If I’m writing a crypto library, I probably want all threads to have no access. If I’m writing a database, I probably want all threads to get RO by default. If I’m writing some doodad to sandbox some carefully constructed code, I might want all threads to have full access by default. —Andy
On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > > > If I recall correctly, the POWER maintainer did express a strong desire > > back then for (what is, I believe) their current semantics, which my > > PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > > Ram, I really really don't like the POWER semantics. Can you give some > justification for them? Does POWER at least have an atomic way for > userspace to modify just the key it wants to modify or, even better, > special load and store instructions to use alternate keys? I wouldn't call it POWER semantics. The way I implemented it on power lead to the semantics, given that nothing was explicitly stated about how the semantics should work within a signal handler. As far as power ISA is concerned, there are no special load and store instructions that can somehow circumvent the permissions of the key associated with the PTE. Also unlike x86; on POWER, pkey-permissions are not administered based on its run context (user context, signal context). Hence the default behavior tends to make the key permissions remain the same regardless of the context. > Does POWER at least have an atomic way for userspace to modify just the > key it wants to modify .. ? No. just like PKRU, on power its a register which has bits corresponding to each key. Entire register has to be read and written, which means multiple keys could get modified in the same write. adding ppc mailing list to the CC. > > --Andy
On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > > > > > If I recall correctly, the POWER maintainer did express a strong desire > > > back then for (what is, I believe) their current semantics, which my > > > PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > > > > Ram, I really really don't like the POWER semantics. Can you give some > > justification for them? Does POWER at least have an atomic way for > > userspace to modify just the key it wants to modify or, even better, > > special load and store instructions to use alternate keys? > I wouldn't call it POWER semantics. The way I implemented it on power > lead to the semantics, given that nothing was explicitly stated > about how the semantics should work within a signal handler. I think that this is further evidence that we should introduce a new pkey_alloc() mode and deprecate the old. To the extent possible, this thing should work the same way on x86 and POWER. I think that we, as kernel API designers enabling fancy hardware features, need to think about them with some care. Our goal isn't just to expose the hardware feature to userspace and let userspace run wild with it -- our goal is to figure out what the use cases are and make the API useful for those use cases without introducing more footguns that necessary. For pkey, this means realizing that user code consists of various loosely coupled components and that the purpose of pkeys is to allow some userspace component to prevent other components from *accidentally* clobbering or leaking data due to bugs. And I think that the current APIs don't really achieve this.
On 05/02/2018 11:23 PM, Andy Lutomirski wrote: >> The kernel could do*something*, probably along the membarrier system >> call. I mean, I could implement a reasonable close approximation in >> userspace, via the setxid mechanism in glibc (but I really don't want to). > I beg to differ. > > Thread A: > old = RDPKRU(); > WRPKRU(old & ~3); > ... > WRPKRU(old); > > Thread B: > pkey_alloc(). > > If pkey_alloc() happens while thread A is in the ... part, you lose. It > makes no difference what the kernel does. The problem is that the WRPKRU > instruction itself is designed incorrectly. Even that is solvable, as long as the architecture as exact traps: You can look at the program counter and patch up the registers accordingly if the code is in the critical section. Of course, this would need centralizing PKRU updates in a vDSO or a single (glibc) library function. Certainly not nice and even horrible enough not to do it, but I don't think it's actually impossible. Didn't we discuss this before? Thanks, Florian
On 05/02/2018 06:14 PM, Andy Lutomirski wrote: >> I think you are saying: If a thread calls pkey_alloc(), all >> threads should, by default, implicitly get access. > No, I’m saying that all threads should get the *requested* access. > If I’m protecting the GOT, I want all threads to get RO access. If > I’m writing a crypto library, I probably want all threads to have no > access. If I’m writing a database, I probably want all threads to > get RO by default. If I’m writing some doodad to sandbox some > carefully constructed code, I might want all threads to have full > access by default. OK, fair enough. I totally agree that the current interface (or architecture for that matter) is not amenable to use models where we are implicitly imposing policies on *other* threads. I don't think that means the current stuff is broken for multi-threading, though, just the (admittedly useful) cases you are talking about where you want to poke at a remote thread's PKRU. So, where do we go from here?
On 05/03/2018 03:14 AM, Andy Lutomirski wrote:
> No, I’m saying that all threads should get the*requested* access. If I’m protecting the GOT, I want all threads to get RO access. If I’m writing a crypto library, I probably want all threads to have no access. If I’m writing a database, I probably want all threads to get RO by default. If I’m writing some doodad to sandbox some carefully constructed code, I might want all threads to have full access by default.
Just a clarification: This key allocation issue is *not* a blocker for
anything related to a safer GOT, or any other use of memory protection
keys by the C implementation itself. I agree that there could be
application issues if threads are created early, but solving this issue
in a general way appears to be quite costly.
Thanks,
Florian
On 05/02/2018 11:18 PM, Andy Lutomirski wrote: > I don't know about POWER's ISA, but this is crappy behavior. If a thread > temporarily grants itself access to a restrictive memory key and then gets > a signal, the signal handler should*not* have access to that key. Sorry, that totally depends on what the signal handler does, especially for synchronously delivered signals. Thanks, Florian
On 05/03/2018 12:03 AM, Dave Hansen wrote: > On 05/02/2018 02:08 PM, Florian Weimer wrote: >> On 05/02/2018 05:28 PM, Dave Hansen wrote: >>> The other option here that I think we discussed in the past was to have >>> an*explicit* signal PKRU value. That way, we can be restrictive by >>> default but allow overrides for special cases like you have. >> >> That's the patch I posted before (with PKEY_ALLOC_SETSIGNAL). I'm >> afraid we are going in circles. > > Could you remind us why you abandoned that approach and its relative > merits versus this new approach? Ram argued for the PKEY_ALLOC_SIGNALINHERIT and no one else objected or was interested at the time. I may have misread the consensus. I'm not sure what do here. I tried to submit patches for the two suggested approaches, and each one resulted in suggests to implement the other semantics instead. Thanks, Florian
On 05/03/2018 01:38 AM, Ram Pai wrote: > This is a new requirement that I was not aware off. Its not documented > anywhere AFAICT. Correct. All inheritance behavior was deliberately left unspecified. I'm surprised about the reluctance to fix the x86 behavior. Are there any applications at all for the current semantics? I guess I can implement this particular glibc hardening on POWER only for now. Thanks, Florian
On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > >> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: >>> >>>> If I recall correctly, the POWER maintainer did express a strong > desire >>>> back then for (what is, I believe) their current semantics, which my >>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. >>> >>> Ram, I really really don't like the POWER semantics. Can you give some >>> justification for them? Does POWER at least have an atomic way for >>> userspace to modify just the key it wants to modify or, even better, >>> special load and store instructions to use alternate keys? > >> I wouldn't call it POWER semantics. The way I implemented it on power >> lead to the semantics, given that nothing was explicitly stated >> about how the semantics should work within a signal handler. > > I think that this is further evidence that we should introduce a new > pkey_alloc() mode and deprecate the old. To the extent possible, this > thing should work the same way on x86 and POWER. Do you propose to change POWER or to change x86? Thanks, Florian
On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote: > On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > > On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > > > >> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > >>> > >>>> If I recall correctly, the POWER maintainer did express a strong > > desire > >>>> back then for (what is, I believe) their current semantics, which my > >>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > >>> > >>> Ram, I really really don't like the POWER semantics. Can you give some > >>> justification for them? Does POWER at least have an atomic way for > >>> userspace to modify just the key it wants to modify or, even better, > >>> special load and store instructions to use alternate keys? > > > >> I wouldn't call it POWER semantics. The way I implemented it on power > >> lead to the semantics, given that nothing was explicitly stated > >> about how the semantics should work within a signal handler. > > > > I think that this is further evidence that we should introduce a new > > pkey_alloc() mode and deprecate the old. To the extent possible, this > > thing should work the same way on x86 and POWER. > Do you propose to change POWER or to change x86? Sorry for being slow to reply. I propose to introduce a new PKEY_ALLOC_something variant on x86 and POWER and to make the behavior match on both. It should at least update the values loaded when a signal is delivered and it should probably also update it for new threads. For glibc, for example, I assume that you want signals to be delivered with write access disabled to the GOT. Otherwise you would fail to protect against exploits that occur in signal context. Glibc controls thread creation, so the initial state on thread startup doesn't really matter, but there will be more users than just glibc. --Andy
On 05/08/2018 04:49 AM, Andy Lutomirski wrote: > On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote: > >> On 05/03/2018 06:05 AM, Andy Lutomirski wrote: >>> On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: >>> >>>> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: >>>>> >>>>>> If I recall correctly, the POWER maintainer did express a strong >>> desire >>>>>> back then for (what is, I believe) their current semantics, which my >>>>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. >>>>> >>>>> Ram, I really really don't like the POWER semantics. Can you give > some >>>>> justification for them? Does POWER at least have an atomic way for >>>>> userspace to modify just the key it wants to modify or, even better, >>>>> special load and store instructions to use alternate keys? >>> >>>> I wouldn't call it POWER semantics. The way I implemented it on power >>>> lead to the semantics, given that nothing was explicitly stated >>>> about how the semantics should work within a signal handler. >>> >>> I think that this is further evidence that we should introduce a new >>> pkey_alloc() mode and deprecate the old. To the extent possible, this >>> thing should work the same way on x86 and POWER. > >> Do you propose to change POWER or to change x86? > > Sorry for being slow to reply. I propose to introduce a new > PKEY_ALLOC_something variant on x86 and POWER and to make the behavior > match on both. So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the existing (different) behavior without the flag? Ram, would you be okay with that? Could you give me a hand if necessary? (I assume we have silicon in-house because it's a long-standing feature of the POWER platform which was simply dormant on Linux until now.) > It should at least update the values loaded when a signal > is delivered and it should probably also update it for new threads. I think we should keep inheritance for new threads and fork. pkey_alloc only has a single access rights argument, which makes it hard to reuse this interface if there are two (three) separate sets of access rights. Is there precedent for process state reverting on fork, besides MADV_WIPEONFORK? My gut feeling is that we should avoid that. > For glibc, for example, I assume that you want signals to be delivered with > write access disabled to the GOT. Otherwise you would fail to protect > against exploits that occur in signal context. Glibc controls thread > creation, so the initial state on thread startup doesn't really matter, but > there will be more users than just glibc. glibc does not control thread, or more precisely, subprocess creation. Otherwise we wouldn't have face that many issues with our PID cache. 8-/ Thanks, Florian
On Tue, May 8, 2018 at 5:40 AM Florian Weimer <fweimer@redhat.com> wrote: > On 05/08/2018 04:49 AM, Andy Lutomirski wrote: > > On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote: > > > >> On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > >>> On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > >>> > >>>> On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > >>>>> > >>>>>> If I recall correctly, the POWER maintainer did express a strong > >>> desire > >>>>>> back then for (what is, I believe) their current semantics, which my > >>>>>> PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > >>>>> > >>>>> Ram, I really really don't like the POWER semantics. Can you give > > some > >>>>> justification for them? Does POWER at least have an atomic way for > >>>>> userspace to modify just the key it wants to modify or, even better, > >>>>> special load and store instructions to use alternate keys? > >>> > >>>> I wouldn't call it POWER semantics. The way I implemented it on power > >>>> lead to the semantics, given that nothing was explicitly stated > >>>> about how the semantics should work within a signal handler. > >>> > >>> I think that this is further evidence that we should introduce a new > >>> pkey_alloc() mode and deprecate the old. To the extent possible, this > >>> thing should work the same way on x86 and POWER. > > > >> Do you propose to change POWER or to change x86? > > > > Sorry for being slow to reply. I propose to introduce a new > > PKEY_ALLOC_something variant on x86 and POWER and to make the behavior > > match on both. > So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the > existing (different) behavior without the flag? > Ram, would you be okay with that? Could you give me a hand if > necessary? (I assume we have silicon in-house because it's a > long-standing feature of the POWER platform which was simply dormant on > Linux until now.) > > It should at least update the values loaded when a signal > > is delivered and it should probably also update it for new threads. > I think we should keep inheritance for new threads and fork. pkey_alloc > only has a single access rights argument, which makes it hard to reuse > this interface if there are two (three) separate sets of access rights. Hmm. I can get on board with the idea that fork() / clone() / pthread_create() are all just special cases of the idea that the thread that *calls* them should have the right pkey values, and the latter is already busted given our inability to asynchronously propagate the new mode in pkey_alloc(). So let's so PKEY_ALLOC_SETSIGNAL as a starting point. One thing we could do, though: the current initual state on process creation is all access blocked on all keys. We could change it so that half the keys are fully blocked and half are read-only. Then we could add a PKEY_ALLOC_STRICT or similar that allocates a key with the correct initial state *and* does the setsignal thing. If there are no keys left with the correct initial state, then it fails.
On 05/09/2018 04:41 PM, Andy Lutomirski wrote: > Hmm. I can get on board with the idea that fork() / clone() / > pthread_create() are all just special cases of the idea that the thread > that*calls* them should have the right pkey values, and the latter is > already busted given our inability to asynchronously propagate the new mode > in pkey_alloc(). So let's so PKEY_ALLOC_SETSIGNAL as a starting point. Ram, any suggestions for implementing this on POWER? > One thing we could do, though: the current initual state on process > creation is all access blocked on all keys. We could change it so that > half the keys are fully blocked and half are read-only. Then we could add > a PKEY_ALLOC_STRICT or similar that allocates a key with the correct > initial state*and* does the setsignal thing. If there are no keys left > with the correct initial state, then it fails. The initial PKRU value can currently be configured by the system administrator. I fear this approach has too many moving parts to be viable. Thanks, Florian
> On May 14, 2018, at 5:01 AM, Florian Weimer <fweimer@redhat.com> wrote: > >> One thing we could do, though: the current initual state on process >> creation is all access blocked on all keys. We could change it so that >> half the keys are fully blocked and half are read-only. Then we could add >> a PKEY_ALLOC_STRICT or similar that allocates a key with the correct >> initial state*and* does the setsignal thing. If there are no keys left >> with the correct initial state, then it fails. > > The initial PKRU value can currently be configured by the system administrator. I fear this approach has too many moving parts to be viable. > > Honestly, I think we should drop that option. I don’t see how we can expect an administrator to do this usefully.
On 05/14/2018 05:32 PM, Andy Lutomirski wrote: > > > >> On May 14, 2018, at 5:01 AM, Florian Weimer <fweimer@redhat.com> wrote: >> >>> One thing we could do, though: the current initual state on process >>> creation is all access blocked on all keys. We could change it so that >>> half the keys are fully blocked and half are read-only. Then we could add >>> a PKEY_ALLOC_STRICT or similar that allocates a key with the correct >>> initial state*and* does the setsignal thing. If there are no keys left >>> with the correct initial state, then it fails. >> >> The initial PKRU value can currently be configured by the system administrator. I fear this approach has too many moving parts to be viable. >> >> > > Honestly, I think we should drop that option. I don’t see how we can expect an administrator to do this usefully. I don't disagree—it makes things way less predictable in practice. Thanks, Florian
On 05/14/2018 08:34 AM, Florian Weimer wrote: >>> The initial PKRU value can currently be configured by the system >>> administrator. I fear this approach has too many moving parts to be >>> viable. >> >> Honestly, I think we should drop that option. I don’t see how we can >> expect an administrator to do this usefully. > > I don't disagree—it makes things way less predictable in practice. I originally put that thing in there to make Andy happy with the initial permissions, and give us a way to back it out if something went wrong. I have no objections to removing it either.
On Tue, May 08, 2018 at 02:40:46PM +0200, Florian Weimer wrote: > On 05/08/2018 04:49 AM, Andy Lutomirski wrote: > >On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote: > > > >>On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > >>>On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > >>> > >>>>On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > >>>>> > >>>>>>If I recall correctly, the POWER maintainer did express a strong > >>>desire > >>>>>>back then for (what is, I believe) their current semantics, which my > >>>>>>PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > >>>>> > >>>>>Ram, I really really don't like the POWER semantics. Can you give > >some > >>>>>justification for them? Does POWER at least have an atomic way for > >>>>>userspace to modify just the key it wants to modify or, even better, > >>>>>special load and store instructions to use alternate keys? > >>> > >>>>I wouldn't call it POWER semantics. The way I implemented it on power > >>>>lead to the semantics, given that nothing was explicitly stated > >>>>about how the semantics should work within a signal handler. > >>> > >>>I think that this is further evidence that we should introduce a new > >>>pkey_alloc() mode and deprecate the old. To the extent possible, this > >>>thing should work the same way on x86 and POWER. > > > >>Do you propose to change POWER or to change x86? > > > >Sorry for being slow to reply. I propose to introduce a new > >PKEY_ALLOC_something variant on x86 and POWER and to make the behavior > >match on both. > > So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the > existing (different) behavior without the flag? > > Ram, would you be okay with that? Could you give me a hand if > necessary? (I assume we have silicon in-house because it's a > long-standing feature of the POWER platform which was simply dormant > on Linux until now.) Yes. I can help you with that. So let me see if I understand the overall idea. Application can allocate new keys through a new syscall sys_pkey_alloc_1(flags, init_val, sig_init_val) 'sig_init_val' is the permission-state of the key in signal context. The kernel will set the permission of each keys to their corresponding values when entering the signal handler and revert on return from the signal handler. just like init_val, sig_init_val also percolates to children threads. Is this a correct understanding? RP
On Wed, May 16, 2018 at 1:35 PM Ram Pai <linuxram@us.ibm.com> wrote: > On Tue, May 08, 2018 at 02:40:46PM +0200, Florian Weimer wrote: > > On 05/08/2018 04:49 AM, Andy Lutomirski wrote: > > >On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> wrote: > > > > > >>On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > > >>>On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > > >>> > > >>>>On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > > >>>>> > > >>>>>>If I recall correctly, the POWER maintainer did express a strong > > >>>desire > > >>>>>>back then for (what is, I believe) their current semantics, which my > > >>>>>>PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > > >>>>> > > >>>>>Ram, I really really don't like the POWER semantics. Can you give > > >some > > >>>>>justification for them? Does POWER at least have an atomic way for > > >>>>>userspace to modify just the key it wants to modify or, even better, > > >>>>>special load and store instructions to use alternate keys? > > >>> > > >>>>I wouldn't call it POWER semantics. The way I implemented it on power > > >>>>lead to the semantics, given that nothing was explicitly stated > > >>>>about how the semantics should work within a signal handler. > > >>> > > >>>I think that this is further evidence that we should introduce a new > > >>>pkey_alloc() mode and deprecate the old. To the extent possible, this > > >>>thing should work the same way on x86 and POWER. > > > > > >>Do you propose to change POWER or to change x86? > > > > > >Sorry for being slow to reply. I propose to introduce a new > > >PKEY_ALLOC_something variant on x86 and POWER and to make the behavior > > >match on both. > > > > So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the > > existing (different) behavior without the flag? > > > > Ram, would you be okay with that? Could you give me a hand if > > necessary? (I assume we have silicon in-house because it's a > > long-standing feature of the POWER platform which was simply dormant > > on Linux until now.) > Yes. I can help you with that. > So let me see if I understand the overall idea. > Application can allocate new keys through a new syscall > sys_pkey_alloc_1(flags, init_val, sig_init_val) > 'sig_init_val' is the permission-state of the key in signal context. > The kernel will set the permission of each keys to their > corresponding values when entering the signal handler and revert > on return from the signal handler. > just like init_val, sig_init_val also percolates to children threads. I was imagining it would be just pkey_alloc(SOME_NEW_FLAG, init_val); and the init val would be used for the current thread and for signal handlers. New threads would inherit their parents' values. The latter is certainly up for negotiation, but it's the simplest behavior, and it's not obvious to be that it's wrong. --Andy
On Mon, May 14, 2018 at 02:01:23PM +0200, Florian Weimer wrote: > On 05/09/2018 04:41 PM, Andy Lutomirski wrote: > >Hmm. I can get on board with the idea that fork() / clone() / > >pthread_create() are all just special cases of the idea that the thread > >that*calls* them should have the right pkey values, and the latter is > >already busted given our inability to asynchronously propagate the new mode > >in pkey_alloc(). So let's so PKEY_ALLOC_SETSIGNAL as a starting point. > > Ram, any suggestions for implementing this on POWER? I suspect the changes will go in restore_user_regs() and save_user_regs(). These are the functions that save and restore register state before entry and exit into/from a signal handler. > > >One thing we could do, though: the current initual state on process > >creation is all access blocked on all keys. We could change it so that > >half the keys are fully blocked and half are read-only. Then we could add > >a PKEY_ALLOC_STRICT or similar that allocates a key with the correct > >initial state*and* does the setsignal thing. If there are no keys left > >with the correct initial state, then it fails. > > The initial PKRU value can currently be configured by the system > administrator. I fear this approach has too many moving parts to be > viable. Sounds like on x86 keys can go active in signal-handler without any explicit allocation request by the application. This is not the case on power. Is that API requirement? Hope not. RP
On Wed, May 16, 2018 at 1:52 PM Ram Pai <linuxram@us.ibm.com> wrote: > On Mon, May 14, 2018 at 02:01:23PM +0200, Florian Weimer wrote: > > On 05/09/2018 04:41 PM, Andy Lutomirski wrote: > > >Hmm. I can get on board with the idea that fork() / clone() / > > >pthread_create() are all just special cases of the idea that the thread > > >that*calls* them should have the right pkey values, and the latter is > > >already busted given our inability to asynchronously propagate the new mode > > >in pkey_alloc(). So let's so PKEY_ALLOC_SETSIGNAL as a starting point. > > > > Ram, any suggestions for implementing this on POWER? > I suspect the changes will go in > restore_user_regs() and save_user_regs(). These are the functions > that save and restore register state before entry and exit into/from > a signal handler. > > > > >One thing we could do, though: the current initual state on process > > >creation is all access blocked on all keys. We could change it so that > > >half the keys are fully blocked and half are read-only. Then we could add > > >a PKEY_ALLOC_STRICT or similar that allocates a key with the correct > > >initial state*and* does the setsignal thing. If there are no keys left > > >with the correct initial state, then it fails. > > > > The initial PKRU value can currently be configured by the system > > administrator. I fear this approach has too many moving parts to be > > viable. > Sounds like on x86 keys can go active in signal-handler > without any explicit allocation request by the application. This is not > the case on power. Is that API requirement? Hope not. On x86, signals are currently delivered with all keys locked all the way down (except for the magic one we use to emulate no-read access). I would hesitate to change this for existing applications.
On Wed, May 16, 2018 at 01:37:46PM -0700, Andy Lutomirski wrote: > On Wed, May 16, 2018 at 1:35 PM Ram Pai <linuxram@us.ibm.com> wrote: > > > On Tue, May 08, 2018 at 02:40:46PM +0200, Florian Weimer wrote: > > > On 05/08/2018 04:49 AM, Andy Lutomirski wrote: > > > >On Mon, May 7, 2018 at 2:48 AM Florian Weimer <fweimer@redhat.com> > wrote: > > > > > > > >>On 05/03/2018 06:05 AM, Andy Lutomirski wrote: > > > >>>On Wed, May 2, 2018 at 7:11 PM Ram Pai <linuxram@us.ibm.com> wrote: > > > >>> > > > >>>>On Wed, May 02, 2018 at 09:23:49PM +0000, Andy Lutomirski wrote: > > > >>>>> > > > >>>>>>If I recall correctly, the POWER maintainer did express a strong > > > >>>desire > > > >>>>>>back then for (what is, I believe) their current semantics, which > my > > > >>>>>>PKEY_ALLOC_SIGNALINHERIT patch implements for x86, too. > > > >>>>> > > > >>>>>Ram, I really really don't like the POWER semantics. Can you give > > > >some > > > >>>>>justification for them? Does POWER at least have an atomic way for > > > >>>>>userspace to modify just the key it wants to modify or, even > better, > > > >>>>>special load and store instructions to use alternate keys? > > > >>> > > > >>>>I wouldn't call it POWER semantics. The way I implemented it on > power > > > >>>>lead to the semantics, given that nothing was explicitly stated > > > >>>>about how the semantics should work within a signal handler. > > > >>> > > > >>>I think that this is further evidence that we should introduce a new > > > >>>pkey_alloc() mode and deprecate the old. To the extent possible, > this > > > >>>thing should work the same way on x86 and POWER. > > > > > > > >>Do you propose to change POWER or to change x86? > > > > > > > >Sorry for being slow to reply. I propose to introduce a new > > > >PKEY_ALLOC_something variant on x86 and POWER and to make the behavior > > > >match on both. > > > > > > So basically implement PKEY_ALLOC_SETSIGNAL for POWER, and keep the > > > existing (different) behavior without the flag? > > > > > > Ram, would you be okay with that? Could you give me a hand if > > > necessary? (I assume we have silicon in-house because it's a > > > long-standing feature of the POWER platform which was simply dormant > > > on Linux until now.) > > > Yes. I can help you with that. > > > So let me see if I understand the overall idea. > > > Application can allocate new keys through a new syscall > > sys_pkey_alloc_1(flags, init_val, sig_init_val) > > > 'sig_init_val' is the permission-state of the key in signal context. > > > The kernel will set the permission of each keys to their > > corresponding values when entering the signal handler and revert > > on return from the signal handler. > > > just like init_val, sig_init_val also percolates to children threads. > > > I was imagining it would be just pkey_alloc(SOME_NEW_FLAG, init_val); and > the init val would be used for the current thread and for signal handlers. what would change the key-permission-values enforced in signal-handler context? Or can it never be changed, ones set through sys_pkey_alloc()? I suppose key-permission-values change done in non-signal-handler context, will not apply to those in signal-handler context. Can the signal handler change the key-permission-values from the signal-handler context? RP
On 05/16/2018 11:07 PM, Ram Pai wrote: > what would change the key-permission-values enforced in signal-handler > context? Or can it never be changed, ones set through sys_pkey_alloc()? The access rights can only be set by pkey_alloc and are unchanged after that (so we do not have to discuss whether the signal handler access rights are per-thread or not). > I suppose key-permission-values change done in non-signal-handler context, > will not apply to those in signal-handler context. Correct, that is the plan. > Can the signal handler change the key-permission-values from the > signal-handler context? Yes, changes are possible. The access rights given to pkey_alloc only specify the initial access rights when the signal handler is entered. We need to decide if we should restore it on exit from the signal handler. There is also the matter of siglongjmp, which currently does not restore the current thread's access rights. In general, this might be difficult to implement because of the limited space in jmp_buf. Thanks, Florian
On 05/16/2018 10:35 PM, Ram Pai wrote: > So let me see if I understand the overall idea. > > Application can allocate new keys through a new syscall > sys_pkey_alloc_1(flags, init_val, sig_init_val) > > 'sig_init_val' is the permission-state of the key in signal context. I would keep the existing system call and just add a flag, say PKEY_ALLOC_SETSIGNAL. If the current thread needs different access rights, it can set those rights just after pkey_alloc returns. There is no race that matters here, I think. Thanks, Florian
diff --git a/Documentation/x86/protection-keys.txt b/Documentation/x86/protection-keys.txt index ecb0d2dadfb7..d46d8e501c3a 100644 --- a/Documentation/x86/protection-keys.txt +++ b/Documentation/x86/protection-keys.txt @@ -39,11 +39,18 @@ with a key. In this example WRPKRU is wrapped by a C function called pkey_set(). int real_prot = PROT_READ|PROT_WRITE; - pkey = pkey_alloc(0, PKEY_DISABLE_WRITE); + pkey = pkey_alloc(PKEY_ALLOC_SIGNALINHERIT, PKEY_DISABLE_WRITE); ptr = mmap(NULL, PAGE_SIZE, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0); ret = pkey_mprotect(ptr, PAGE_SIZE, real_prot, pkey); ... application runs here +The PKEY_ALLOC_SIGNALINHERIT flag ensures the that key allocation +fails if the kernel does not support access rights inheritance for +signal handlers. (Some kernel versions implement different semantics +where signal handlers execute not with the access rights of the +interrupted thread, but with some unspecified system default access +rights.) + Now, if the application needs to update the data at 'ptr', it can gain access, do the update, then remove its write access: diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h index f9d4e6b6d4bd..39468bf388a2 100644 --- a/arch/alpha/include/uapi/asm/mman.h +++ b/arch/alpha/include/uapi/asm/mman.h @@ -73,6 +73,8 @@ /* compatibility flags */ #define MAP_FILE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h index 3035ca499cd8..dfe6cd82403a 100644 --- a/arch/mips/include/uapi/asm/mman.h +++ b/arch/mips/include/uapi/asm/mman.h @@ -100,6 +100,8 @@ /* compatibility flags */ #define MAP_FILE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h index 870fbf8c7088..773c130c17db 100644 --- a/arch/parisc/include/uapi/asm/mman.h +++ b/arch/parisc/include/uapi/asm/mman.h @@ -70,6 +70,8 @@ #define MAP_FILE 0 #define MAP_VARIABLE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h index a38bf5a1e37a..a87e99f72be6 100644 --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -33,6 +33,7 @@ extern int fpu__restore_sig(void __user *buf, int ia32_frame); extern void fpu__drop(struct fpu *fpu); extern int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu); extern void fpu__clear(struct fpu *fpu); +extern void fpu__clear_signal(struct fpu *fpu); extern int fpu__exception_code(struct fpu *fpu, int trap_nr); extern int dump_fpu(struct pt_regs *ptregs, struct user_i387_struct *fpstate); diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c index f92a6593de1e..a3b304888af8 100644 --- a/arch/x86/kernel/fpu/core.c +++ b/arch/x86/kernel/fpu/core.c @@ -370,21 +370,16 @@ static inline void copy_init_fpstate_to_fpregs(void) copy_kernel_to_fxregs(&init_fpstate.fxsave); else copy_kernel_to_fregs(&init_fpstate.fsave); - - if (boot_cpu_has(X86_FEATURE_OSPKE)) - copy_init_pkru_to_fpregs(); } -/* - * Clear the FPU state back to init state. - * - * Called by sys_execve(), by the signal handler code and by various - * error paths. - */ -void fpu__clear(struct fpu *fpu) +static void __fpu_clear(struct fpu *fpu, bool for_signal) { + u32 pkru; + WARN_ON_FPU(fpu != ¤t->thread.fpu); /* Almost certainly an anomaly */ + if (for_signal) + pkru = read_pkru(); fpu__drop(fpu); /* @@ -395,10 +390,42 @@ void fpu__clear(struct fpu *fpu) fpu__initialize(fpu); user_fpu_begin(); copy_init_fpstate_to_fpregs(); + if (boot_cpu_has(X86_FEATURE_OSPKE)) { + /* A signal handler inherits the original PKRU + * value of the interrupted thread. + */ + if (for_signal) + __write_pkru(pkru); + else + copy_init_pkru_to_fpregs(); + } preempt_enable(); } } +/* + * Clear the FPU state back to init state. + * + * Called by sys_execve(), the signal handler return code, and by + * various error paths. + */ +void fpu__clear(struct fpu *fpu) +{ + return __fpu_clear(fpu, false); +} + +/* + * Prepare the FPU for invoking a signal handler. + * + * This is like fpu__clear(), but some CPU registers are inherited + * from the current thread and not restored to their initial values, + * to match behavior on other architectures. + */ +void fpu__clear_signal(struct fpu *fpu) +{ + return __fpu_clear(fpu, true); +} + /* * x87 math exception handling: */ diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index da270b95fe4d..b3c1f6f3df66 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -758,7 +758,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs) * Ensure the signal handler starts with the new fpu state. */ if (fpu->initialized) - fpu__clear(fpu); + fpu__clear_signal(fpu); } signal_setup_done(failed, ksig, stepping); } diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h index 58f29a9d895d..741b5d39882f 100644 --- a/arch/xtensa/include/uapi/asm/mman.h +++ b/arch/xtensa/include/uapi/asm/mman.h @@ -112,6 +112,8 @@ /* compatibility flags */ #define MAP_FILE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h index e7ee32861d51..18f6c1ebe2bb 100644 --- a/include/uapi/asm-generic/mman-common.h +++ b/include/uapi/asm-generic/mman-common.h @@ -69,6 +69,8 @@ /* compatibility flags */ #define MAP_FILE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/mm/mprotect.c b/mm/mprotect.c index 625608bc8962..ec82728774af 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -539,14 +539,21 @@ SYSCALL_DEFINE4(pkey_mprotect, unsigned long, start, size_t, len, return do_mprotect_pkey(start, len, prot, pkey); } +#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SIGNALINHERIT)) + SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val) { int pkey; int ret; - /* No flags supported yet. */ - if (flags) + /* Check for unsupported flags. No further action for + * PKEY_ALLOC_SIGNALINHERIT is required; this flag merely + * provides a way for applications to detect that allocated + * keys support inheriting access rights in signal handler. + */ + if (flags & ~PKEY_ALLOC_FLAGS) return -EINVAL; + /* check for unsupported init values */ if (init_val & ~PKEY_ACCESS_MASK) return -EINVAL; diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h index e7ee32861d51..18f6c1ebe2bb 100644 --- a/tools/include/uapi/asm-generic/mman-common.h +++ b/tools/include/uapi/asm-generic/mman-common.h @@ -69,6 +69,8 @@ /* compatibility flags */ #define MAP_FILE 0 +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ diff --git a/tools/testing/selftests/x86/protection_keys.c b/tools/testing/selftests/x86/protection_keys.c index f15aa5a76fe3..e651c83e09aa 100644 --- a/tools/testing/selftests/x86/protection_keys.c +++ b/tools/testing/selftests/x86/protection_keys.c @@ -393,6 +393,8 @@ pid_t fork_lazy_child(void) return forkret; } +#define PKEY_ALLOC_SIGNALINHERIT 0x1 + #define PKEY_DISABLE_ACCESS 0x1 #define PKEY_DISABLE_WRITE 0x2 @@ -560,7 +562,7 @@ int alloc_pkey(void) dprintf1("alloc_pkey()::%d, pkru: 0x%x shadow: %x\n", __LINE__, __rdpkru(), shadow_pkru); - ret = sys_pkey_alloc(0, init_val); + ret = sys_pkey_alloc(PKEY_ALLOC_SIGNALINHERIT, init_val); /* * pkey_alloc() sets PKRU, so we need to reflect it in * shadow_pkru:
pkeys support for IBM POWER intends to inherited the access rights of the current thread in signal handlers. The advantage is that this preserves access to memory regions associated with non-default keys, enabling additional usage scenarios for memory protection keys which currently do not work on x86 due to the unconditional reset to the (configurable) default key in signal handlers. Consequently, this commit updates the x86 implementation to preserve the PKRU register value of the interrupted context in signal handlers. If a key is allocated successfully with the PKEY_ALLOC_SIGNALINHERIT flag, the application can assume this signal inheritance behavior. This change does not affect the init_pkru optimization because if the thread's PKRU register is zero due to the init_pkru setting, it will remain zero in the signal handler through inheritance from the interrupted context. After this change, this program: ??=include <sys/syscall.h> ??=include <unistd.h> ??=include <stdio.h> ??=include <err.h> ??=include <signal.h> ??=define PKEY_ALLOC_SIGNALINHERIT 1 ??=define PKEY_DISABLE_ACCESS 1 ??=define PKEY_DISABLE_WRITE 2 static inline unsigned int pkey_read (void) { unsigned int result; __asm__ volatile (".byte 0x0f, 0x01, 0xee" : "=a" (result) : "c" (0) : "rdx"); return result; } static void print_pkru (const char *where) { printf ("PKRU (%s): %08x\n", where, pkey_read ()); } static void sigusr1 (int signo) { print_pkru ("signal handler"); } int main (void) { if (signal (SIGUSR1, sigusr1) == SIG_ERR) err (1, "signal"); print_pkru ("main"); raise (SIGUSR1); puts ("allocating key 1"); int key1 = syscall (SYS_pkey_alloc, PKEY_ALLOC_SIGNALINHERIT, 0); if (key1 < 0) err (1, "pkey_alloc"); print_pkru ("main"); raise (SIGUSR1); puts ("allocating key 2"); int key2 = syscall (SYS_pkey_alloc, PKEY_ALLOC_SIGNALINHERIT, PKEY_DISABLE_ACCESS); if (key2 < 0) err (1, "pkey_alloc"); print_pkru ("main"); raise (SIGUSR1); puts ("allocating key 3"); int key3 = syscall (SYS_pkey_alloc, PKEY_ALLOC_SIGNALINHERIT, PKEY_DISABLE_WRITE); if (key3 < 0) err (1, "pkey_alloc"); print_pkru ("main"); raise (SIGUSR1); return 0; } should print: PKRU (main): 55555554 PKRU (signal handler): 55555554 allocating key 1 PKRU (main): 55555550 PKRU (signal handler): 55555550 allocating key 2 PKRU (main): 55555550 PKRU (signal handler): 55555550 allocating key 3 PKRU (main): 55555590 PKRU (signal handler): 55555590 That is, the PKRU register value in the signal handler matches that of the main thread, even if the access rights have been changed from the system default. Signed-off-by: Florian Weimer <fweimer@redhat.com> --- Documentation/x86/protection-keys.txt | 9 ++++- arch/alpha/include/uapi/asm/mman.h | 2 ++ arch/mips/include/uapi/asm/mman.h | 2 ++ arch/parisc/include/uapi/asm/mman.h | 2 ++ arch/x86/include/asm/fpu/internal.h | 1 + arch/x86/kernel/fpu/core.c | 47 +++++++++++++++++++++------ arch/x86/kernel/signal.c | 2 +- arch/xtensa/include/uapi/asm/mman.h | 2 ++ include/uapi/asm-generic/mman-common.h | 2 ++ mm/mprotect.c | 11 +++++-- tools/include/uapi/asm-generic/mman-common.h | 2 ++ tools/testing/selftests/x86/protection_keys.c | 4 ++- 12 files changed, 71 insertions(+), 15 deletions(-)