| Message ID | b8d6ae4483fcfd17524e79d803c969694a85cc02.1687428075.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9a32584bc108c8fe4d02fa33b16caf686e4a788a |
| Headers | show |
| Series | [v2] powerpc/ptrace: Split gpr32_set_common | 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_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > objtool report the following warning: > > arch/powerpc/kernel/ptrace/ptrace-view.o: warning: objtool: > gpr32_set_common+0x23c (.text+0x860): redundant UACCESS disable > > gpr32_set_common() conditionnaly opens and closes UACCESS based on > whether kbuf point is NULL or not. This is wackelig. > > Split gpr32_set_common() in two fonctions, one for user one for > kernel. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > v2: Mark gpr32_set_common_kernel() and gpr32_set_common_user() static > --- > arch/powerpc/kernel/ptrace/ptrace-view.c | 106 ++++++++++++++--------- > 1 file changed, 67 insertions(+), 39 deletions(-) > > diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c > index 3910cd7bb2d9..42abbed452cd 100644 > --- a/arch/powerpc/kernel/ptrace/ptrace-view.c > +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c > @@ -716,73 +716,89 @@ int gpr32_get_common(struct task_struct *target, > return membuf_zero(&to, (ELF_NGREG - PT_REGS_COUNT) * sizeof(u32)); > } > > -int gpr32_set_common(struct task_struct *target, > - const struct user_regset *regset, > - unsigned int pos, unsigned int count, > - const void *kbuf, const void __user *ubuf, > - unsigned long *regs) > +static int gpr32_set_common_kernel(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, unsigned long *regs) > { > const compat_ulong_t *k = kbuf; > + > + pos /= sizeof(compat_ulong_t); > + count /= sizeof(compat_ulong_t); > + > + for (; count > 0 && pos < PT_MSR; --count) > + regs[pos++] = *k++; > + > + if (count > 0 && pos == PT_MSR) { > + set_user_msr(target, *k++); > + ++pos; > + --count; > + } > + > + for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) > + regs[pos++] = *k++; > + for (; count > 0 && pos < PT_TRAP; --count, ++pos) > + ++k; > + > + if (count > 0 && pos == PT_TRAP) { > + set_user_trap(target, *k++); > + ++pos; > + --count; > + } > + > + kbuf = k; > + pos *= sizeof(compat_ulong_t); > + count *= sizeof(compat_ulong_t); > + user_regset_copyin_ignore(&pos, &count, &kbuf, NULL, > + (PT_TRAP + 1) * sizeof(compat_ulong_t), -1); > + return 0; > +} > + > +static int gpr32_set_common_user(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void __user *ubuf, unsigned long *regs) > +{ > const compat_ulong_t __user *u = ubuf; > compat_ulong_t reg; > > - if (!kbuf && !user_read_access_begin(u, count)) > + if (!user_read_access_begin(u, count)) > return -EFAULT; > > pos /= sizeof(reg); > count /= sizeof(reg); > > - if (kbuf) > - for (; count > 0 && pos < PT_MSR; --count) > - regs[pos++] = *k++; > - else > - for (; count > 0 && pos < PT_MSR; --count) { > - unsafe_get_user(reg, u++, Efault); > - regs[pos++] = reg; > - } > - > + for (; count > 0 && pos < PT_MSR; --count) { > + unsafe_get_user(reg, u++, Efault); > + regs[pos++] = reg; > + } > > if (count > 0 && pos == PT_MSR) { > - if (kbuf) > - reg = *k++; > - else > - unsafe_get_user(reg, u++, Efault); > + unsafe_get_user(reg, u++, Efault); > set_user_msr(target, reg); > ++pos; > --count; > } > > - if (kbuf) { > - for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) > - regs[pos++] = *k++; > - for (; count > 0 && pos < PT_TRAP; --count, ++pos) > - ++k; > - } else { > - for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) { > - unsafe_get_user(reg, u++, Efault); > - regs[pos++] = reg; > - } > - for (; count > 0 && pos < PT_TRAP; --count, ++pos) > - unsafe_get_user(reg, u++, Efault); > + for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) { > + unsafe_get_user(reg, u++, Efault); > + regs[pos++] = reg; > } > + for (; count > 0 && pos < PT_TRAP; --count, ++pos) > + unsafe_get_user(reg, u++, Efault); > > if (count > 0 && pos == PT_TRAP) { > - if (kbuf) > - reg = *k++; > - else > - unsafe_get_user(reg, u++, Efault); > + unsafe_get_user(reg, u++, Efault); > set_user_trap(target, reg); > ++pos; > --count; > } > - if (!kbuf) > - user_read_access_end(); > + user_read_access_end(); > > - kbuf = k; > ubuf = u; > pos *= sizeof(reg); > count *= sizeof(reg); > - user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, > + user_regset_copyin_ignore(&pos, &count, NULL, &ubuf, > (PT_TRAP + 1) * sizeof(reg), -1); > return 0; This was oopsing: [ 1508.081530][T16432] BUG: Kernel NULL pointer dereference on read at 0x00000000 [ 1508.081551][T16432] Faulting instruction address: 0xc00000000002c690 [ 1508.081558][T16432] Oops: Kernel access of bad area, sig: 11 [#2] [ 1508.081565][T16432] BE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=64 NUMA pSeries [ 1508.081573][T16432] Modules linked in: [ 1508.081580][T16432] CPU: 0 PID: 16432 Comm: ptrace-gpr Tainted: G D 6.5.0-rc3-00069-gb23dade91efd #1 [ 1508.081589][T16432] Hardware name: IBM,9117-MMA POWER6 (raw) 0x3e0301 0xf000002 of:IBM,EM350_176 hv:phyp pSeries [ 1508.081597][T16432] NIP: c00000000002c690 LR: c00000000002f0f0 CTR: 0000000000000000 [ 1508.081604][T16432] REGS: c00000002000ba80 TRAP: 0300 Tainted: G D (6.5.0-rc3-00069-gb23dade91efd) [ 1508.081612][T16432] MSR: 8000000000009032 <SF,EE,ME,IR,DR,RI> CR: 24004224 XER: 00000000 [ 1508.081632][T16432] CFAR: c00000000002c82c DAR: 0000000000000000 DSISR: 40000000 IRQMASK: 0 [ 1508.081632][T16432] GPR00: c00000000002f0f0 c00000002000bd20 c000000001347e00 c00000002116ab80 [ 1508.081632][T16432] GPR04: 0000000000000005 0000000010030270 000000000000002d c00000002aa3bfb0 [ 1508.081632][T16432] GPR08: c00000002aa3be80 0000000000000000 0000000000000000 0000000000000000 [ 1508.081632][T16432] GPR12: c00000000002d370 c0000000019f0000 0000000000000000 0000000000000000 [ 1508.081632][T16432] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 1508.081632][T16432] GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 1508.081632][T16432] GPR24: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 1508.081632][T16432] GPR28: 00000000100301d0 00000000100301d0 000000000000000c 00000000000000a4 [ 1508.081735][T16432] NIP [c00000000002c690] gpr32_set_common_user.isra.0+0x160/0x490 [ 1508.081748][T16432] LR [c00000000002f0f0] compat_arch_ptrace+0x4a0/0xaf0 [ 1508.081756][T16432] Call Trace: [ 1508.081760][T16432] [c00000002000bd20] [c00000002000bd60] 0xc00000002000bd60 (unreliable) [ 1508.081771][T16432] [c00000002000bd50] [c00000000002f0f0] compat_arch_ptrace+0x4a0/0xaf0 [ 1508.081781][T16432] [c00000002000bdc0] [c000000000176734] compat_sys_ptrace+0x174/0x1e0 [ 1508.081791][T16432] [c00000002000be10] [c00000000002b404] system_call_exception+0x374/0x380 [ 1508.081803][T16432] [c00000002000be50] [c00000000000cb54] system_call_common+0xf4/0x258 Because user_regset_copyin_ignore() always dereferences kbuf: static inline void user_regset_copyin_ignore(unsigned int *pos, unsigned int *count, const void **kbuf, const void __user **ubuf, const int start_pos, const int end_pos) { if (*count == 0) return; BUG_ON(*pos < start_pos); if (end_pos < 0 || *pos < end_pos) { unsigned int copy = (end_pos < 0 ? *count : min(*count, end_pos - *pos)); if (*kbuf) *kbuf += copy; I fixed it with: diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c index 42abbed452cd..584cf5c3df50 100644 --- a/arch/powerpc/kernel/ptrace/ptrace-view.c +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c @@ -760,6 +760,7 @@ static int gpr32_set_common_user(struct task_struct *target, const void __user *ubuf, unsigned long *regs) { const compat_ulong_t __user *u = ubuf; + const void *kbuf = NULL; compat_ulong_t reg; if (!user_read_access_begin(u, count)) @@ -798,7 +799,7 @@ static int gpr32_set_common_user(struct task_struct *target, ubuf = u; pos *= sizeof(reg); count *= sizeof(reg); - user_regset_copyin_ignore(&pos, &count, NULL, &ubuf, + user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, (PT_TRAP + 1) * sizeof(reg), -1); return 0; cheers
On Thu, 22 Jun 2023 12:01:23 +0200, Christophe Leroy wrote: > objtool report the following warning: > > arch/powerpc/kernel/ptrace/ptrace-view.o: warning: objtool: > gpr32_set_common+0x23c (.text+0x860): redundant UACCESS disable > > gpr32_set_common() conditionnaly opens and closes UACCESS based on > whether kbuf point is NULL or not. This is wackelig. > > [...] Applied to powerpc/next. [1/1] powerpc/ptrace: Split gpr32_set_common https://git.kernel.org/powerpc/c/9a32584bc108c8fe4d02fa33b16caf686e4a788a cheers
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c index 3910cd7bb2d9..42abbed452cd 100644 --- a/arch/powerpc/kernel/ptrace/ptrace-view.c +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c @@ -716,73 +716,89 @@ int gpr32_get_common(struct task_struct *target, return membuf_zero(&to, (ELF_NGREG - PT_REGS_COUNT) * sizeof(u32)); } -int gpr32_set_common(struct task_struct *target, - const struct user_regset *regset, - unsigned int pos, unsigned int count, - const void *kbuf, const void __user *ubuf, - unsigned long *regs) +static int gpr32_set_common_kernel(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, unsigned long *regs) { const compat_ulong_t *k = kbuf; + + pos /= sizeof(compat_ulong_t); + count /= sizeof(compat_ulong_t); + + for (; count > 0 && pos < PT_MSR; --count) + regs[pos++] = *k++; + + if (count > 0 && pos == PT_MSR) { + set_user_msr(target, *k++); + ++pos; + --count; + } + + for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) + regs[pos++] = *k++; + for (; count > 0 && pos < PT_TRAP; --count, ++pos) + ++k; + + if (count > 0 && pos == PT_TRAP) { + set_user_trap(target, *k++); + ++pos; + --count; + } + + kbuf = k; + pos *= sizeof(compat_ulong_t); + count *= sizeof(compat_ulong_t); + user_regset_copyin_ignore(&pos, &count, &kbuf, NULL, + (PT_TRAP + 1) * sizeof(compat_ulong_t), -1); + return 0; +} + +static int gpr32_set_common_user(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void __user *ubuf, unsigned long *regs) +{ const compat_ulong_t __user *u = ubuf; compat_ulong_t reg; - if (!kbuf && !user_read_access_begin(u, count)) + if (!user_read_access_begin(u, count)) return -EFAULT; pos /= sizeof(reg); count /= sizeof(reg); - if (kbuf) - for (; count > 0 && pos < PT_MSR; --count) - regs[pos++] = *k++; - else - for (; count > 0 && pos < PT_MSR; --count) { - unsafe_get_user(reg, u++, Efault); - regs[pos++] = reg; - } - + for (; count > 0 && pos < PT_MSR; --count) { + unsafe_get_user(reg, u++, Efault); + regs[pos++] = reg; + } if (count > 0 && pos == PT_MSR) { - if (kbuf) - reg = *k++; - else - unsafe_get_user(reg, u++, Efault); + unsafe_get_user(reg, u++, Efault); set_user_msr(target, reg); ++pos; --count; } - if (kbuf) { - for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) - regs[pos++] = *k++; - for (; count > 0 && pos < PT_TRAP; --count, ++pos) - ++k; - } else { - for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) { - unsafe_get_user(reg, u++, Efault); - regs[pos++] = reg; - } - for (; count > 0 && pos < PT_TRAP; --count, ++pos) - unsafe_get_user(reg, u++, Efault); + for (; count > 0 && pos <= PT_MAX_PUT_REG; --count) { + unsafe_get_user(reg, u++, Efault); + regs[pos++] = reg; } + for (; count > 0 && pos < PT_TRAP; --count, ++pos) + unsafe_get_user(reg, u++, Efault); if (count > 0 && pos == PT_TRAP) { - if (kbuf) - reg = *k++; - else - unsafe_get_user(reg, u++, Efault); + unsafe_get_user(reg, u++, Efault); set_user_trap(target, reg); ++pos; --count; } - if (!kbuf) - user_read_access_end(); + user_read_access_end(); - kbuf = k; ubuf = u; pos *= sizeof(reg); count *= sizeof(reg); - user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, + user_regset_copyin_ignore(&pos, &count, NULL, &ubuf, (PT_TRAP + 1) * sizeof(reg), -1); return 0; @@ -791,6 +807,18 @@ int gpr32_set_common(struct task_struct *target, return -EFAULT; } +int gpr32_set_common(struct task_struct *target, + const struct user_regset *regset, + unsigned int pos, unsigned int count, + const void *kbuf, const void __user *ubuf, + unsigned long *regs) +{ + if (kbuf) + return gpr32_set_common_kernel(target, regset, pos, count, kbuf, regs); + else + return gpr32_set_common_user(target, regset, pos, count, ubuf, regs); +} + static int gpr32_get(struct task_struct *target, const struct user_regset *regset, struct membuf to)
objtool report the following warning: arch/powerpc/kernel/ptrace/ptrace-view.o: warning: objtool: gpr32_set_common+0x23c (.text+0x860): redundant UACCESS disable gpr32_set_common() conditionnaly opens and closes UACCESS based on whether kbuf point is NULL or not. This is wackelig. Split gpr32_set_common() in two fonctions, one for user one for kernel. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- v2: Mark gpr32_set_common_kernel() and gpr32_set_common_user() static --- arch/powerpc/kernel/ptrace/ptrace-view.c | 106 ++++++++++++++--------- 1 file changed, 67 insertions(+), 39 deletions(-)