| Message ID | d5ef83c361cc255494afd15ff1b4fb02a36e1dcf.1641468127.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc/bpf: Some fixes and updates | expand |
On 1/6/22 12:45 PM, Naveen N. Rao wrote: > task_pt_regs() can return NULL on powerpc for kernel threads. This is > then used in __bpf_get_stack() to check for user mode, resulting in a > kernel oops. Guard against this by checking return value of > task_pt_regs() before trying to obtain the call chain. > > Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()") > Cc: stable@vger.kernel.org # v5.9+ > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Le 06/01/2022 à 12:45, Naveen N. Rao a écrit : > task_pt_regs() can return NULL on powerpc for kernel threads. This is > then used in __bpf_get_stack() to check for user mode, resulting in a > kernel oops. Guard against this by checking return value of > task_pt_regs() before trying to obtain the call chain. I started looking at that some time ago, and I'm wondering whether it is worth keeping that powerpc particularity. We used to have a potentially different pt_regs depending on how we entered kernel, especially on PPC32, but since the following commits it is not the case anymore. 06d67d54741a ("powerpc: make process.c suitable for both 32-bit and 64-bit") db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry") b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE") We could therefore just do like other architectures, define #define task_pt_regs(p) ((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1) And then remove the regs field we have in thread_struct. > > Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()") > Cc: stable@vger.kernel.org # v5.9+ > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > --- > kernel/bpf/stackmap.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c > index 6e75bbee39f0b5..0dcaed4d3f4cec 100644 > --- a/kernel/bpf/stackmap.c > +++ b/kernel/bpf/stackmap.c > @@ -525,13 +525,14 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf, > u32, size, u64, flags) > { > struct pt_regs *regs; > - long res; > + long res = -EINVAL; > > if (!try_get_task_stack(task)) > return -EFAULT; > > regs = task_pt_regs(task); > - res = __bpf_get_stack(regs, task, NULL, buf, size, flags); > + if (regs) > + res = __bpf_get_stack(regs, task, NULL, buf, size, flags); Should there be a comment explaining that on powerpc, 'regs' can be NULL for a kernel thread ? > put_task_stack(task); > > return res;
Christophe Leroy wrote: > > > Le 06/01/2022 à 12:45, Naveen N. Rao a écrit : >> task_pt_regs() can return NULL on powerpc for kernel threads. This is >> then used in __bpf_get_stack() to check for user mode, resulting in a >> kernel oops. Guard against this by checking return value of >> task_pt_regs() before trying to obtain the call chain. > > I started looking at that some time ago, and I'm wondering whether it is > worth keeping that powerpc particularity. > > We used to have a potentially different pt_regs depending on how we > entered kernel, especially on PPC32, but since the following commits it > is not the case anymore. > > 06d67d54741a ("powerpc: make process.c suitable for both 32-bit and 64-bit") > db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry") > b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE") > > We could therefore just do like other architectures, define > > #define task_pt_regs(p) ((struct pt_regs *)(THREAD_SIZE + > task_stack_page(p)) - 1) > > And then remove the regs field we have in thread_struct. Sure, I don't have an opinion on that, but I think this patch will still be needed for -stable. > > >> >> Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()") >> Cc: stable@vger.kernel.org # v5.9+ >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> --- >> kernel/bpf/stackmap.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c >> index 6e75bbee39f0b5..0dcaed4d3f4cec 100644 >> --- a/kernel/bpf/stackmap.c >> +++ b/kernel/bpf/stackmap.c >> @@ -525,13 +525,14 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf, >> u32, size, u64, flags) >> { >> struct pt_regs *regs; >> - long res; >> + long res = -EINVAL; >> >> if (!try_get_task_stack(task)) >> return -EFAULT; >> >> regs = task_pt_regs(task); >> - res = __bpf_get_stack(regs, task, NULL, buf, size, flags); >> + if (regs) >> + res = __bpf_get_stack(regs, task, NULL, buf, size, flags); > > Should there be a comment explaining that on powerpc, 'regs' can be NULL > for a kernel thread ? I guess this won't be required if we end up with the change you are proposing above? - Naveen
diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c index 6e75bbee39f0b5..0dcaed4d3f4cec 100644 --- a/kernel/bpf/stackmap.c +++ b/kernel/bpf/stackmap.c @@ -525,13 +525,14 @@ BPF_CALL_4(bpf_get_task_stack, struct task_struct *, task, void *, buf, u32, size, u64, flags) { struct pt_regs *regs; - long res; + long res = -EINVAL; if (!try_get_task_stack(task)) return -EFAULT; regs = task_pt_regs(task); - res = __bpf_get_stack(regs, task, NULL, buf, size, flags); + if (regs) + res = __bpf_get_stack(regs, task, NULL, buf, size, flags); put_task_stack(task); return res;
task_pt_regs() can return NULL on powerpc for kernel threads. This is then used in __bpf_get_stack() to check for user mode, resulting in a kernel oops. Guard against this by checking return value of task_pt_regs() before trying to obtain the call chain. Fixes: fa28dcb82a38f8 ("bpf: Introduce helper bpf_get_task_stack()") Cc: stable@vger.kernel.org # v5.9+ Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> --- kernel/bpf/stackmap.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)