| Message ID | 20211130072933.2004389-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] powerpc/signal: sanitise PT_NIP and sa_handler low bits | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > I'm not entirely sure about the 32-bit / compat part. Or the 64-bit for > that matter except that it does seem to fix the bug caused by the test > program. > > Thanks, > Nick > > arch/powerpc/kernel/signal_32.c | 23 ++++++++++++++++------- > arch/powerpc/kernel/signal_64.c | 17 ++++++++++++----- > 2 files changed, 28 insertions(+), 12 deletions(-) Sorry for the delayed feedback. I was observing confusing test results so had to be sure. Test results are against 5.16.0-rc5-03218-g798527287598 (powerpc/merge) I ran some extended set of kernel self tests (from powerpc/signal and powerpc/security) on POWER8, POWER9 and POWER10 configs. On POWER8 & POWER10 LPAR with this fix tests ran successfully. on POWER9 PowerNV with the fix and following set of configs CONFIG_PPC_IRQ_SOFT_MASK_DEBUG=y CONFIG_PPC_RFI_SRR_DEBUG=y the tests ran successfully. But with the fix and following set of configs CONFIG_PPC_IRQ_SOFT_MASK_DEBUG=y CONFIG_PPC_RFI_SRR_DEBUG=n I still a warning and then a crash: [ 550.568588] count-cache-flush: hardware flush enabled. [ 550.568593] link-stack-flush: software flush enabled. [ 550.569604] ------------[ cut here ]------------ [ 550.569611] WARNING: CPU: 21 PID: 3784 at arch/powerpc/kernel/irq.c:288 arch_local_irq_restore+0x22c/0x230 [ 550.569625] Modules linked in: nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables rfkill libcrc32c nfnetlink i2c_dev rpcrdma sunrpc ib_umad rdma_ucm ib_srpt ib_isert iscsi_target_mod target_core_mod ib_ipoib ib_iser rdma_cm iw_cm libiscsi ib_cm scsi_transport_iscsi mlx5_ib ib_uverbs dm_mod ib_core ses enclosure tpm_i2c_nuvoton i2c_opal ipmi_powernv xts ipmi_devintf uio_pdrv_genirq vmx_crypto ipmi_msghandler i2c_core opal_prd uio ibmpowernv leds_powernv powernv_op_panel sch_fq_codel ip_tables ext4 mbcache jbd2 mlx5_core sd_mod t10_pi sg mpt3sas ipr tg3 libata mlxfw psample raid_class ptp scsi_transport_sas pps_core fuse [ 550.569752] CPU: 21 PID: 3784 Comm: NetworkManager Kdump: loaded Not tainted 5.16.0-rc5-03218-g798527287598 #8 [ 550.569765] NIP: c0000000000171dc LR: c000000000033240 CTR: c000000000cf1260 [ 550.569774] REGS: c000000ae08bbab0 TRAP: 0700 Not tainted (5.16.0-rc5-03218-g798527287598) [ 550.569784] MSR: 9000000000021031 <SF,HV,ME,IR,DR,LE> CR: 28004444 XER: 20040000 [ 550.569802] CFAR: c00000000001704c IRQMASK: 1 [ 550.569802] GPR00: c000000000033240 c000000ae08bbd50 c000000002a10600 0000000000000000 [ 550.569802] GPR04: c000000ae08bbe80 00007fffaea1ece0 0000000000000000 0000000000000000 [ 550.569802] GPR08: 0000000000000002 0000000000000000 0000000000000002 0000000001f3fb0c [ 550.569802] GPR12: 0000000000004000 c000005fff7c9080 0000000000000000 0000000000000000 [ 550.569802] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.569802] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.569802] GPR24: 0000000000000002 0000000000000001 0000000002002000 0000000002802000 [ 550.569802] GPR28: 0000000000000000 0000000000000800 c000000ae08bbe80 0000000000040080 [ 550.569899] NIP [c0000000000171dc] arch_local_irq_restore+0x22c/0x230 [ 550.569909] LR [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 [ 550.569919] Call Trace: [ 550.569925] [c000000ae08bbd80] [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 [ 550.569937] [c000000ae08bbde0] [c000000000033744] syscall_exit_prepare+0x74/0x150 [ 550.569948] [c000000ae08bbe10] [c00000000000c758] system_call_common+0xf8/0x268 [ 550.569960] --- interrupt: c00 at 0x7fffaea1ece0 [ 550.569968] NIP: 00007fffaea1ece0 LR: 00007fffaea1ecc4 CTR: 0000000000000000 [ 550.569977] REGS: c000000ae08bbe80 TRAP: 0c00 Not tainted (5.16.0-rc5-03218-g798527287598) [ 550.569987] MSR: 900000000280f033 <SF,HV,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 24004448 XER: 00000000 [ 550.570012] IRQMASK: 0 [ 550.570012] GPR00: 00000000000000a7 00007fffeea71ce0 00007fffaeb07300 0000000000000001 [ 550.570012] GPR04: 0000000000000007 0000000000013eed 0000000000000000 0000000000000002 [ 550.570012] GPR08: 00007fffad6c7ea8 0000000000000000 0000000000000000 0000000000000000 [ 550.570012] GPR12: 0000000000000000 00007fffad6cf510 0000000000000000 0000000000000000 [ 550.570012] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.570012] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.570012] GPR24: 0000000000000000 0000000000000001 0000000000013eed 00007fffeea71da4 [ 550.570012] GPR28: 0000000000000000 0000000000000007 000000013a1ae810 0000000000013eed [ 550.570105] NIP [00007fffaea1ece0] 0x7fffaea1ece0 [ 550.570112] LR [00007fffaea1ecc4] 0x7fffaea1ecc4 [ 550.570119] --- interrupt: c00 [ 550.570124] Instruction dump: [ 550.570130] f8010040 0fe00000 4bfffff0 60000000 60000000 0fe00000 60000000 60000000 [ 550.570148] 60000000 39200002 7d210164 4bfffec4 <0fe00000> 3c4c02a0 38429420 7c0802a6 [ 550.570165] ---[ end trace b8833ddd6f9d2d40 ]--- [ 550.570174] Unrecoverable exception 700 at c000000000017050 (msr=9000000000021031) [ 550.570184] Oops: Unrecoverable exception, sig: 6 [#1] [ 550.570191] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA PowerNV [ 550.570200] Modules linked in: nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables rfkill libcrc32c nfnetlink i2c_dev rpcrdma sunrpc ib_umad rdma_ucm ib_srpt ib_isert iscsi_target_mod target_core_mod ib_ipoib ib_iser rdma_cm iw_cm libiscsi ib_cm scsi_transport_iscsi mlx5_ib ib_uverbs dm_mod ib_core ses enclosure tpm_i2c_nuvoton i2c_opal ipmi_powernv xts ipmi_devintf uio_pdrv_genirq vmx_crypto ipmi_msghandler i2c_core opal_prd uio ibmpowernv leds_powernv powernv_op_panel sch_fq_codel ip_tables ext4 mbcache jbd2 mlx5_core sd_mod t10_pi sg mpt3sas ipr tg3 libata mlxfw psample raid_class ptp scsi_transport_sas pps_core fuse [ 550.570313] CPU: 21 PID: 3784 Comm: NetworkManager Kdump: loaded Tainted: G W 5.16.0-rc5-03218-g798527287598 #8 [ 550.570326] NIP: c000000000017050 LR: c000000000033240 CTR: c000000000cf1260 [ 550.570335] REGS: c000000ae08bbab0 TRAP: 0700 Tainted: G W (5.16.0-rc5-03218-g798527287598) [ 550.570346] MSR: 9000000000021031 <SF,HV,ME,IR,DR,LE> CR: 28004444 XER: 20040000 [ 550.570363] CFAR: c00000000001704c IRQMASK: 1 [ 550.570363] GPR00: c000000000033240 c000000ae08bbd50 c000000002a10600 0000000000000000 [ 550.570363] GPR04: c000000ae08bbe80 00007fffaea1ece0 0000000000000000 0000000000000000 [ 550.570363] GPR08: 0000000000000002 0000000000000000 0000000000000002 0000000001f3fb0c [ 550.570363] GPR12: 0000000000004000 c000005fff7c9080 0000000000000000 0000000000000000 [ 550.570363] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.570363] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 550.570363] GPR24: 0000000000000002 0000000000000001 0000000002002000 0000000002802000 [ 550.570363] GPR28: 0000000000000000 0000000000000800 c000000ae08bbe80 0000000000040080 …….. Not sure if the above problem is related to the previous one I reported or a different one. Thanks -Sachin
Excerpts from Sachin Sant's message of December 15, 2021 8:49 pm: > >> Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> I'm not entirely sure about the 32-bit / compat part. Or the 64-bit for >> that matter except that it does seem to fix the bug caused by the test >> program. >> >> Thanks, >> Nick >> >> arch/powerpc/kernel/signal_32.c | 23 ++++++++++++++++------- >> arch/powerpc/kernel/signal_64.c | 17 ++++++++++++----- >> 2 files changed, 28 insertions(+), 12 deletions(-) > > Sorry for the delayed feedback. I was observing confusing test results > so had to be sure. > > Test results are against 5.16.0-rc5-03218-g798527287598 (powerpc/merge) > > I ran some extended set of kernel self tests (from powerpc/signal and > powerpc/security) on POWER8, POWER9 and POWER10 configs. > > On POWER8 & POWER10 LPAR with this fix tests ran successfully. > > on POWER9 PowerNV with the fix and following set of configs > > CONFIG_PPC_IRQ_SOFT_MASK_DEBUG=y > CONFIG_PPC_RFI_SRR_DEBUG=y > > the tests ran successfully. > > But with the fix and following set of configs > > CONFIG_PPC_IRQ_SOFT_MASK_DEBUG=y > CONFIG_PPC_RFI_SRR_DEBUG=n > > I still a warning and then a crash: > > [ 550.568588] count-cache-flush: hardware flush enabled. > [ 550.568593] link-stack-flush: software flush enabled. > [ 550.569604] ------------[ cut here ]------------ > [ 550.569611] WARNING: CPU: 21 PID: 3784 at arch/powerpc/kernel/irq.c:288 arch_local_irq_restore+0x22c/0x230 > [ 550.569625] Modules linked in: nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables rfkill libcrc32c nfnetlink i2c_dev rpcrdma sunrpc ib_umad rdma_ucm ib_srpt ib_isert iscsi_target_mod target_core_mod ib_ipoib ib_iser rdma_cm iw_cm libiscsi ib_cm scsi_transport_iscsi mlx5_ib ib_uverbs dm_mod ib_core ses enclosure tpm_i2c_nuvoton i2c_opal ipmi_powernv xts ipmi_devintf uio_pdrv_genirq vmx_crypto ipmi_msghandler i2c_core opal_prd uio ibmpowernv leds_powernv powernv_op_panel sch_fq_codel ip_tables ext4 mbcache jbd2 mlx5_core sd_mod t10_pi sg mpt3sas ipr tg3 libata mlxfw psample raid_class ptp scsi_transport_sas pps_core fuse > [ 550.569752] CPU: 21 PID: 3784 Comm: NetworkManager Kdump: loaded Not tainted 5.16.0-rc5-03218-g798527287598 #8 > [ 550.569765] NIP: c0000000000171dc LR: c000000000033240 CTR: c000000000cf1260 > [ 550.569774] REGS: c000000ae08bbab0 TRAP: 0700 Not tainted (5.16.0-rc5-03218-g798527287598) > [ 550.569784] MSR: 9000000000021031 <SF,HV,ME,IR,DR,LE> CR: 28004444 XER: 20040000 > [ 550.569802] CFAR: c00000000001704c IRQMASK: 1 > [ 550.569802] GPR00: c000000000033240 c000000ae08bbd50 c000000002a10600 0000000000000000 > [ 550.569802] GPR04: c000000ae08bbe80 00007fffaea1ece0 0000000000000000 0000000000000000 > [ 550.569802] GPR08: 0000000000000002 0000000000000000 0000000000000002 0000000001f3fb0c > [ 550.569802] GPR12: 0000000000004000 c000005fff7c9080 0000000000000000 0000000000000000 > [ 550.569802] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > [ 550.569802] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 > [ 550.569802] GPR24: 0000000000000002 0000000000000001 0000000002002000 0000000002802000 > [ 550.569802] GPR28: 0000000000000000 0000000000000800 c000000ae08bbe80 0000000000040080 > [ 550.569899] NIP [c0000000000171dc] arch_local_irq_restore+0x22c/0x230 > [ 550.569909] LR [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 > [ 550.569919] Call Trace: > [ 550.569925] [c000000ae08bbd80] [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 > [ 550.569937] [c000000ae08bbde0] [c000000000033744] syscall_exit_prepare+0x74/0x150 > [ 550.569948] [c000000ae08bbe10] [c00000000000c758] system_call_common+0xf8/0x268 Yeah this looks like a different issue. Is there a test running which flips the security mitigations rapidly? There is a race window with the the static branch causing exit_must_hard_disable() returning two different values. We should update they key while single threaded AFAIKS. Thanks, Nick --- diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c index 57c6bb802f6c..a7cb317e7039 100644 --- a/arch/powerpc/lib/feature-fixups.c +++ b/arch/powerpc/lib/feature-fixups.c @@ -232,11 +232,22 @@ static DEFINE_MUTEX(exit_flush_lock); static int __do_stf_barrier_fixups(void *data) { - enum stf_barrier_type *types = data; + enum stf_barrier_type types = *(enum stf_barrier_type *)data; do_stf_entry_barrier_fixups(*types); do_stf_exit_barrier_fixups(*types); + if ((types & STF_BARRIER_FALLBACK) || (types & STF_BARRIER_SYNC_ORI)) + stf_exit_reentrant = false; + else + stf_exit_reentrant = true; + + if (stf_exit_reentrant && rfi_exit_reentrant) + static_branch_disable(&interrupt_exit_not_reentrant); + else + static_branch_enable(&interrupt_exit_not_reentrant); + + return 0; } @@ -257,18 +268,9 @@ void do_stf_barrier_fixups(enum stf_barrier_type types) // Prevent static key update races with do_rfi_flush_fixups() mutex_lock(&exit_flush_lock); - static_branch_enable(&interrupt_exit_not_reentrant); stop_machine(__do_stf_barrier_fixups, &types, NULL); - if ((types & STF_BARRIER_FALLBACK) || (types & STF_BARRIER_SYNC_ORI)) - stf_exit_reentrant = false; - else - stf_exit_reentrant = true; - - if (stf_exit_reentrant && rfi_exit_reentrant) - static_branch_disable(&interrupt_exit_not_reentrant); - mutex_unlock(&exit_flush_lock); } @@ -472,6 +474,16 @@ static int __do_rfi_flush_fixups(void *data) patch_instruction(dest + 2, ppc_inst(instrs[2])); } + if (types & L1D_FLUSH_FALLBACK) + rfi_exit_reentrant = false; + else + rfi_exit_reentrant = true; + + if (stf_exit_reentrant && rfi_exit_reentrant) + static_branch_disable(&interrupt_exit_not_reentrant); + else + static_branch_enable(&interrupt_exit_not_reentrant); + printk(KERN_DEBUG "rfi-flush: patched %d locations (%s flush)\n", i, (types == L1D_FLUSH_NONE) ? "no" : (types == L1D_FLUSH_FALLBACK) ? "fallback displacement" : @@ -495,18 +507,9 @@ void do_rfi_flush_fixups(enum l1d_flush_type types) // Prevent static key update races with do_stf_barrier_fixups() mutex_lock(&exit_flush_lock); - static_branch_enable(&interrupt_exit_not_reentrant); stop_machine(__do_rfi_flush_fixups, &types, NULL); - if (types & L1D_FLUSH_FALLBACK) - rfi_exit_reentrant = false; - else - rfi_exit_reentrant = true; - - if (stf_exit_reentrant && rfi_exit_reentrant) - static_branch_disable(&interrupt_exit_not_reentrant); - mutex_unlock(&exit_flush_lock); }
>> [ 550.569802] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 >> [ 550.569802] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 >> [ 550.569802] GPR24: 0000000000000002 0000000000000001 0000000002002000 0000000002802000 >> [ 550.569802] GPR28: 0000000000000000 0000000000000800 c000000ae08bbe80 0000000000040080 >> [ 550.569899] NIP [c0000000000171dc] arch_local_irq_restore+0x22c/0x230 >> [ 550.569909] LR [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 >> [ 550.569919] Call Trace: >> [ 550.569925] [c000000ae08bbd80] [c000000000033240] interrupt_exit_user_prepare_main+0x150/0x260 >> [ 550.569937] [c000000ae08bbde0] [c000000000033744] syscall_exit_prepare+0x74/0x150 >> [ 550.569948] [c000000ae08bbe10] [c00000000000c758] system_call_common+0xf8/0x268 > > Yeah this looks like a different issue. Is there a test running which > flips the security mitigations rapidly? There is a race window with Yes, powerpc/security/mitigation-patching.sh. This test enables/disables various supported mitigations (parallel execution). > the the static branch causing exit_must_hard_disable() returning two > different values. > > We should update they key while single threaded AFAIKS. Thanks. I tested with this fix. The test ran correctly without a crash. > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c > index 57c6bb802f6c..a7cb317e7039 100644 > --- a/arch/powerpc/lib/feature-fixups.c > +++ b/arch/powerpc/lib/feature-fixups.c > @@ -232,11 +232,22 @@ static DEFINE_MUTEX(exit_flush_lock); > > static int __do_stf_barrier_fixups(void *data) > { > - enum stf_barrier_type *types = data; > + enum stf_barrier_type types = *(enum stf_barrier_type *)data; > > do_stf_entry_barrier_fixups(*types); > do_stf_exit_barrier_fixups(*types); > *types should be changed to “types” to avoid build failure.
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c index 3e053e2fd6b6..5379bece8072 100644 --- a/arch/powerpc/kernel/signal_32.c +++ b/arch/powerpc/kernel/signal_32.c @@ -116,7 +116,7 @@ __unsafe_restore_general_regs(struct pt_regs *regs, struct mcontext __user *sr) int i; for (i = 0; i <= PT_RESULT; i++) { - if ((i == PT_MSR) || (i == PT_SOFTE)) + if ((i == PT_NIP) || (i == PT_MSR) || (i == PT_SOFTE)) continue; unsafe_get_user(gregs[i], &sr->mc_gregs[i], failed); } @@ -156,7 +156,7 @@ static __always_inline int __unsafe_restore_general_regs(struct pt_regs *regs, struct mcontext __user *sr) { /* copy up to but not including MSR */ - unsafe_copy_from_user(regs, &sr->mc_gregs, PT_MSR * sizeof(elf_greg_t), failed); + unsafe_copy_from_user(regs, &sr->mc_gregs, PT_NIP * sizeof(elf_greg_t), failed); /* copy from orig_r3 (the word after the MSR) up to the end */ unsafe_copy_from_user(®s->orig_gpr3, &sr->mc_gregs[PT_ORIG_R3], @@ -458,7 +458,7 @@ static long restore_user_regs(struct pt_regs *regs, struct mcontext __user *sr, int sig) { unsigned int save_r2 = 0; - unsigned long msr; + unsigned long nip, msr; #ifdef CONFIG_VSX int i; #endif @@ -473,6 +473,9 @@ static long restore_user_regs(struct pt_regs *regs, save_r2 = (unsigned int)regs->gpr[2]; unsafe_restore_general_regs(regs, sr, failed); set_trap_norestart(regs); + unsafe_get_user(nip, &sr->mc_gregs[PT_NIP], failed); + nip &= ~3UL; + regs_set_return_ip(regs, nip); unsafe_get_user(msr, &sr->mc_gregs[PT_MSR], failed); if (!sig) regs->gpr[2] = (unsigned long) save_r2; @@ -560,7 +563,7 @@ static long restore_tm_user_regs(struct pt_regs *regs, struct mcontext __user *sr, struct mcontext __user *tm_sr) { - unsigned long msr, msr_hi; + unsigned long nip, msr, msr_hi; int i; if (tm_suspend_disabled) @@ -576,7 +579,9 @@ static long restore_tm_user_regs(struct pt_regs *regs, return 1; unsafe_restore_general_regs(¤t->thread.ckpt_regs, sr, failed); - unsafe_get_user(current->thread.tm_tfhar, &sr->mc_gregs[PT_NIP], failed); + unsafe_get_user(nip, &sr->mc_gregs[PT_NIP], failed); + nip &= ~3UL; + current->thread.tm_tfhar = nip; unsafe_get_user(msr, &sr->mc_gregs[PT_MSR], failed); /* Restore the previous little-endian mode */ @@ -646,6 +651,10 @@ static long restore_tm_user_regs(struct pt_regs *regs, current->thread.used_vsr = true; } + unsafe_get_user(nip, &tm_sr->mc_gregs[PT_NIP], failed); + nip &= ~3UL; + regs_set_return_ip(regs, nip); + /* Get the top half of the MSR from the user context */ unsafe_get_user(msr_hi, &tm_sr->mc_gregs[PT_MSR], failed); msr_hi <<= 32; @@ -801,7 +810,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset, regs->gpr[4] = (unsigned long)&frame->info; regs->gpr[5] = (unsigned long)&frame->uc; regs->gpr[6] = (unsigned long)frame; - regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler); + regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler & ~3UL); /* enter the signal handler in native-endian mode */ regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE)); @@ -889,7 +898,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset, regs->gpr[1] = newsp; regs->gpr[3] = ksig->sig; regs->gpr[4] = (unsigned long) sc; - regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler); + regs_set_return_ip(regs, (unsigned long) ksig->ka.sa.sa_handler & ~3UL); /* enter the signal handler in native-endian mode */ regs_set_return_msr(regs, (regs->msr & ~MSR_LE) | (MSR_KERNEL & MSR_LE)); diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index d1e1fc0acbea..5ef24adb9803 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -336,7 +336,7 @@ static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_ elf_vrreg_t __user *v_regs; #endif unsigned long save_r13 = 0; - unsigned long msr; + unsigned long nip, msr; struct pt_regs *regs = tsk->thread.regs; #ifdef CONFIG_VSX int i; @@ -350,7 +350,9 @@ static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_ /* copy the GPRs */ unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr), efault_out); - unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out); + unsafe_get_user(nip, &sc->gp_regs[PT_NIP], efault_out); + nip &= ~3UL; + regs_set_return_ip(regs, nip); /* get MSR separately, transfer the LE bit if doing signal return */ unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out); if (sig) @@ -434,7 +436,7 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, elf_vrreg_t __user *v_regs, *tm_v_regs; #endif unsigned long err = 0; - unsigned long msr; + unsigned long nip, msr; struct pt_regs *regs = tsk->thread.regs; #ifdef CONFIG_VSX int i; @@ -458,8 +460,13 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, * For the case of getting a signal and simply returning from it, * we don't need to re-copy them here. */ - err |= __get_user(regs->nip, &tm_sc->gp_regs[PT_NIP]); - err |= __get_user(tsk->thread.tm_tfhar, &sc->gp_regs[PT_NIP]); + err |= __get_user(nip, &tm_sc->gp_regs[PT_NIP]); + nip &= ~3UL; + regs_set_return_ip(regs, nip); + + err |= __get_user(nip, &sc->gp_regs[PT_NIP]); + nip &= ~3UL; + tsk->thread.tm_tfhar = nip; /* get MSR separately, transfer the LE bit if doing signal return */ err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
The bottom 2 bits of NIP are ignored when RFI returns with SRR0 = NIP, so regs->nip does not correspond to the actual return address if either of those bits are set. Further, these bits are reserved in SRR0 so they should not be set. Sanitize PT_NIP from signal handlers to ensure they can't be set by userspace, this also keeps the low 2 bit of TFHAR clear, which are similarly reserved. 32-bit signal delivery returns directly to the handler, so sa_handler is sanitised similarly there. This can cause a bug when CONFIG_PPC_RFI_SRR_DEBUG=y on a processor that does not implement the 2 low bits of SRR0 (always read back 0) because SRR0 will not match regs->nip. This was caught by sigfuz, but a simple reproducer follows. #include <stdlib.h> #include <signal.h> #include <ucontext.h> static void trap_signal_handler(int signo, siginfo_t *si, void *uc) { ucontext_t *ucp = uc; ucp->uc_mcontext.gp_regs[PT_NIP] |= 3; } int main(void) { struct sigaction trap_sa; trap_sa.sa_flags = SA_SIGINFO; trap_sa.sa_sigaction = trap_signal_handler; sigaction(SIGUSR1, &trap_sa, NULL); raise(SIGUSR1); exit(EXIT_SUCCESS); } Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- I'm not entirely sure about the 32-bit / compat part. Or the 64-bit for that matter except that it does seem to fix the bug caused by the test program. Thanks, Nick arch/powerpc/kernel/signal_32.c | 23 ++++++++++++++++------- arch/powerpc/kernel/signal_64.c | 17 ++++++++++++----- 2 files changed, 28 insertions(+), 12 deletions(-)