| Message ID | 20241029144539.111155-1-kevin.brodsky@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve arm64 pkeys handling in signal delivery | expand |
On Tue, 29 Oct 2024 14:45:34 +0000, Kevin Brodsky wrote: > This series is a follow-up to Joey's Permission Overlay Extension (POE) > series [1] that recently landed on mainline. The goal is to improve the > way we handle the register that governs which pkeys/POIndex are > accessible (POR_EL0) during signal delivery. As things stand, we may > unexpectedly fail to write the signal frame on the stack because POR_EL0 > is not reset before the uaccess operations. See patch 1 for more details > and the main changes this series brings. > > [...] Applied first patch to arm64 (for-next/fixes), thanks! [1/5] arm64: signal: Improve POR_EL0 handling to avoid uaccess failures https://git.kernel.org/arm64/c/2e8a1acea859 Cheers,
+Kees and Jorge and Jann On Tue, Oct 29, 2024 at 7:45 AM Kevin Brodsky <kevin.brodsky@arm.com> wrote: > > This series is a follow-up to Joey's Permission Overlay Extension (POE) > series [1] that recently landed on mainline. The goal is to improve the > way we handle the register that governs which pkeys/POIndex are > accessible (POR_EL0) during signal delivery. As things stand, we may > unexpectedly fail to write the signal frame on the stack because POR_EL0 > is not reset before the uaccess operations. See patch 1 for more details > and the main changes this series brings. > > A similar series landed recently for x86/MPK [2]; the present series > aims at aligning arm64 with x86. Worth noting: once the signal frame is > written, POR_EL0 is still set to POR_EL0_INIT, granting access to pkey 0 > only. This means that a program that sets up an alternate signal stack > with a non-zero pkey will need some assembly trampoline to set POR_EL0 > before invoking the real signal handler, as discussed here [3]. This is > not ideal, but it makes experimentation with pkeys in signal handlers > possible while waiting for a potential interface to control the pkey > state when delivering a signal. See Pierre's reply [4] for more > information about use-cases and a potential interface. > Apologize in advance that I'm unfamiliar with ARM's POR, up to review this patch series, so I might ask silly questions or based on wrong understanding. It seems that the patch has the same logic as Aruna Ramakrishna proposed for X86, is this correct ? In the latest version of x86 change [1], I have a comment if we want to consider adding a new flag SS_PKEYALTSTACK (see SS_AUTODISARM as an example) in sigaltstack, and restrict this mechanism (overwriting PKRU/POR_EL0 and sigframe) to sigaltstack() with SS_PKEYALTSTACK. There is a subtle difference if we do that, i.e. the existing signaling handling user might not care or do not use PKEY/POE, and overwriting PKRU/POR_EL0 and sigframe every time will add extra CPU time on the signaling delivery, which could be real-time sensitive. Since I raised this comment on X86, I think raising it for ARM for discussion would be ok, it might make sense to have consistent API experience for arm/x86 here. Thanks -Jeff [1] https://lore.kernel.org/lkml/CABi2SkWjF2Sicrr71=a6H8XJyf9q9L_Nd5FPp0CJ2mvB46Rrrg@mail.gmail.com/ > The x86 series also added kselftests to ensure that no spurious SIGSEGV > occurs during signal delivery regardless of which pkey is accessible at > the point where the signal is delivered. This series adapts those > kselftests to allow running them on arm64 (patch 4-5). There is a > dependency on Yury's PKEY_UNRESTRICTED patch [7] for patch 4 > specifically. > > Finally patch 2 is a clean-up following feedback on Joey's series [5]. > > I have tested this series on arm64 and x86_64 (booting and running the > protection_keys and pkey_sighandler_tests mm kselftests). > > - Kevin > > --- > > v2..v3: > * Reordered patches (patch 1 is now the main patch). > * Patch 1: compute por_enable_all with an explicit loop, based on > arch_max_pkey() (suggestion from Dave M). > * Patch 4: improved naming, replaced global pkey reg value with inline > helper, made use of Yury's PKEY_UNRESTRICTED macro [7] (suggestions > from Dave H). > > v2: https://lore.kernel.org/linux-arm-kernel/20241023150511.3923558-1-kevin.brodsky@arm.com/ > > v1..v2: > * In setup_rt_frame(), ensured that POR_EL0 is reset to its original > value if we fail to deliver the signal (addresses Catalin's concern [6]). > * Renamed *unpriv_access* to *user_access* in patch 3 (suggestion from > Dave). > * Made what patch 1-2 do explicit in the commit message body (suggestion > from Dave). > > v1: https://lore.kernel.org/linux-arm-kernel/20241017133909.3837547-1-kevin.brodsky@arm.com/ > > [1] https://lore.kernel.org/linux-arm-kernel/20240822151113.1479789-1-joey.gouly@arm.com/ > [2] https://lore.kernel.org/lkml/20240802061318.2140081-1-aruna.ramakrishna@oracle.com/ > [3] https://lore.kernel.org/lkml/CABi2SkWxNkP2O7ipkP67WKz0-LV33e5brReevTTtba6oKUfHRw@mail.gmail.com/ > [4] https://lore.kernel.org/linux-arm-kernel/87plns8owh.fsf@arm.com/ > [5] https://lore.kernel.org/linux-arm-kernel/20241015114116.GA19334@willie-the-truck/ > [6] https://lore.kernel.org/linux-arm-kernel/Zw6D2waVyIwYE7wd@arm.com/ > [7] https://lore.kernel.org/all/20241028090715.509527-2-yury.khrustalev@arm.com/ > > Cc: akpm@linux-foundation.org > Cc: anshuman.khandual@arm.com > Cc: aruna.ramakrishna@oracle.com > Cc: broonie@kernel.org > Cc: catalin.marinas@arm.com > Cc: dave.hansen@linux.intel.com > Cc: Dave.Martin@arm.com > Cc: jeffxu@chromium.org > Cc: joey.gouly@arm.com > Cc: keith.lucas@oracle.com > Cc: pierre.langlois@arm.com > Cc: shuah@kernel.org > Cc: sroettger@google.com > Cc: tglx@linutronix.de > Cc: will@kernel.org > Cc: yury.khrustalev@arm.com > Cc: linux-kselftest@vger.kernel.org > Cc: x86@kernel.org > > Kevin Brodsky (5): > arm64: signal: Improve POR_EL0 handling to avoid uaccess failures > arm64: signal: Remove unnecessary check when saving POE state > arm64: signal: Remove unused macro > selftests/mm: Use generic pkey register manipulation > selftests/mm: Enable pkey_sighandler_tests on arm64 > > arch/arm64/kernel/signal.c | 95 ++++++++++++--- > tools/testing/selftests/mm/Makefile | 8 +- > tools/testing/selftests/mm/pkey-arm64.h | 1 + > tools/testing/selftests/mm/pkey-x86.h | 2 + > .../selftests/mm/pkey_sighandler_tests.c | 115 ++++++++++++++---- > 5 files changed, 176 insertions(+), 45 deletions(-) > > -- > 2.43.0 >
On 30/10/2024 22:59, Jeff Xu wrote: > Apologize in advance that I'm unfamiliar with ARM's POR, up to review > this patch series, so I might ask silly questions or based on wrong > understanding. That's no problem, your input is very welcome! There is no fundamental difference between POR and PKRU AFAIK - the encoding is different, but the principle is the same. The main thing to keep in mind is that POE (the arm64 extension) allows restricting execution in addition to read/write. > It seems that the patch has the same logic as Aruna Ramakrishna > proposed for X86, is this correct ? Yes, patch 1 aims at aligning arm64 with x86 (same behaviour). Going forward I think we should try and keep the arm64 and x86 handling of pkeys as consistent as possible. > In the latest version of x86 change [1], I have a comment if we want > to consider adding a new flag SS_PKEYALTSTACK (see SS_AUTODISARM as an > example) in sigaltstack, and restrict this mechanism (overwriting > PKRU/POR_EL0 and sigframe) to sigaltstack() with SS_PKEYALTSTACK. > There is a subtle difference if we do that, i.e. the existing > signaling handling user might not care or do not use PKEY/POE, and > overwriting PKRU/POR_EL0 and sigframe every time will add extra CPU > time on the signaling delivery, which could be real-time sensitive. From a purely functional perspective, resetting POR to allow access to all pkeys before writing the signal frame should be safe in any context, and allows keeping the handling simple (no conditional code). The performance aspect is a fair point though, as we are adding an ISB (synchronisation barrier) on the signal delivery path if POE is supported. > Since I raised this comment on X86, I think raising it for ARM for > discussion would be ok, > it might make sense to have consistent API experience for arm/x86 here. And indeed this is what I think is most important at this point. Considering that Aruna's series resets PKRU unconditionally (sigaltstack or not) and has already been pulled into mainline during 6.12-rc1 [2], I still believe that patch 1 is doing the right thing, i.e. aligning arm64 with x86. If the concern with performance is confirmed, and there is an agreement to reset POR/PKRU less eagerly on both architectures, this could potentially be revisited. - Kevin [2] https://lore.kernel.org/lkml/172656199227.2471820.13578261908219597067.tglx@xen13/ > [1] https://lore.kernel.org/lkml/CABi2SkWjF2Sicrr71=a6H8XJyf9q9L_Nd5FPp0CJ2mvB46Rrrg@mail.gmail.com/
On Thu, Oct 31, 2024 at 6:00 AM Kevin Brodsky <kevin.brodsky@arm.com> wrote: > > On 30/10/2024 22:59, Jeff Xu wrote: > > Apologize in advance that I'm unfamiliar with ARM's POR, up to review > > this patch series, so I might ask silly questions or based on wrong > > understanding. > > That's no problem, your input is very welcome! There is no fundamental > difference between POR and PKRU AFAIK - the encoding is different, but > the principle is the same. The main thing to keep in mind is that POE > (the arm64 extension) allows restricting execution in addition to > read/write. > > > It seems that the patch has the same logic as Aruna Ramakrishna > > proposed for X86, is this correct ? > > Yes, patch 1 aims at aligning arm64 with x86 (same behaviour). Going > forward I think we should try and keep the arm64 and x86 handling of > pkeys as consistent as possible. > > > In the latest version of x86 change [1], I have a comment if we want > > to consider adding a new flag SS_PKEYALTSTACK (see SS_AUTODISARM as an > > example) in sigaltstack, and restrict this mechanism (overwriting > > PKRU/POR_EL0 and sigframe) to sigaltstack() with SS_PKEYALTSTACK. > > There is a subtle difference if we do that, i.e. the existing > > signaling handling user might not care or do not use PKEY/POE, and > > overwriting PKRU/POR_EL0 and sigframe every time will add extra CPU > > time on the signaling delivery, which could be real-time sensitive. > > From a purely functional perspective, resetting POR to allow access to > all pkeys before writing the signal frame should be safe in any context, > and allows keeping the handling simple (no conditional code). The > performance aspect is a fair point though, as we are adding an ISB > (synchronisation barrier) on the signal delivery path if POE is supported. > Yes. The functional level is the same. Having worked on a read-time system a bit in the past, I'm aware that signaling handling paths are real-time sensitive. > > Since I raised this comment on X86, I think raising it for ARM for > > discussion would be ok, > > it might make sense to have consistent API experience for arm/x86 here. > > And indeed this is what I think is most important at this point. > Considering that Aruna's series resets PKRU unconditionally (sigaltstack > or not) and has already been pulled into mainline during 6.12-rc1 [2], I > still believe that patch 1 is doing the right thing, i.e. aligning arm64 > with x86. If the concern with performance is confirmed, and there is an > agreement to reset POR/PKRU less eagerly on both architectures, this > could potentially be revisited. > Oh, I didn't know it was already in main. My information is out-dated. It does feel a little rushed because my comment on the performance perspective isn't addressed/responded. -Jeff > - Kevin > > [2] > https://lore.kernel.org/lkml/172656199227.2471820.13578261908219597067.tglx@xen13/ > > > [1] https://lore.kernel.org/lkml/CABi2SkWjF2Sicrr71=a6H8XJyf9q9L_Nd5FPp0CJ2mvB46Rrrg@mail.gmail.com/ >