From: "Steven Rostedt (VMware)" <[email protected]>
Andy had some concerns about using regs_get_kernel_stack_nth() in a new
function regs_get_kernel_argument() as if there's any error in the stack
code, it could cause a bad memory access. To be on the safe side, call
probe_kernel_read() on the stack address to be extra careful in accessing
the memory. A helper function, regs_get_kernel_stack_nth_addr(), was added
to just return the stack address (or NULL if not on the stack), that will be
used to find the address (and could be used by other functions) and read the
address with kernel_probe_read().
Link: http://lkml.kernel.org/r/CALCETrXn9zKTb9i1LP3qoFcpqZHF34BdkuZ5D3N0uCmRr+VnbA@mail.gmail.com
Requested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
---
Changes since v1:
- Make regs_get_kernel_stack_nth() not fault, and not have a
separate function. Only tracing uses it anyway.
arch/x86/include/asm/ptrace.h | 43 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index c2304b25e2fd..055f632ce711 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -237,23 +237,52 @@ static inline int regs_within_kernel_stack(struct pt_regs *regs,
}
/**
+ * regs_get_kernel_stack_nth_addr() - get the address of the Nth entry on stack
+ * @regs: pt_regs which contains kernel stack pointer.
+ * @n: stack entry number.
+ *
+ * regs_get_kernel_stack_nth() returns the address of the @n th entry of the
+ * kernel stack which is specified by @regs. If the @n th entry is NOT in
+ * the kernel stack, this returns NULL.
+ */
+static inline unsigned long *regs_get_kernel_stack_nth_addr(struct pt_regs *regs,
+ unsigned int n)
+{
+ unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
+
+ addr += n;
+ if (regs_within_kernel_stack(regs, (unsigned long)addr))
+ return addr;
+ else
+ return NULL;
+}
+
+/* To avoid include hell, we can't include uaccess.h */
+extern long probe_kernel_read(void *dst, const void *src, size_t size);
+
+/**
* regs_get_kernel_stack_nth() - get Nth entry of the stack
* @regs: pt_regs which contains kernel stack pointer.
* @n: stack entry number.
*
* regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
+ * is specified by @regs. If the @n th entry is NOT in the kernel stack
* this returns 0.
*/
static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
unsigned int n)
{
- unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
- addr += n;
- if (regs_within_kernel_stack(regs, (unsigned long)addr))
- return *addr;
- else
- return 0;
+ unsigned long *addr;
+ unsigned long val;
+ long ret;
+
+ addr = regs_get_kernel_stack_nth_addr(regs, n);
+ if (addr) {
+ ret = probe_kernel_read(&val, addr, sizeof(val));
+ if (!ret)
+ return val;
+ }
+ return 0;
}
/**
--
2.13.6
On Wed, Oct 17, 2018 at 04:59:51PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <[email protected]>
>
> Andy had some concerns about using regs_get_kernel_stack_nth() in a new
> function regs_get_kernel_argument() as if there's any error in the stack
> code, it could cause a bad memory access. To be on the safe side, call
> probe_kernel_read() on the stack address to be extra careful in accessing
> the memory. A helper function, regs_get_kernel_stack_nth_addr(), was added
> to just return the stack address (or NULL if not on the stack), that will be
> used to find the address (and could be used by other functions) and read the
> address with kernel_probe_read().
>
> Link: http://lkml.kernel.org/r/CALCETrXn9zKTb9i1LP3qoFcpqZHF34BdkuZ5D3N0uCmRr+VnbA@mail.gmail.com
> Requested-by: Andy Lutomirski <[email protected]>
> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
> ---
Reviewed-by: Joel Fernandes (Google) <[email protected]>
thanks,
- Joel
> Changes since v1:
>
> - Make regs_get_kernel_stack_nth() not fault, and not have a
> separate function. Only tracing uses it anyway.
>
> arch/x86/include/asm/ptrace.h | 43 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index c2304b25e2fd..055f632ce711 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -237,23 +237,52 @@ static inline int regs_within_kernel_stack(struct pt_regs *regs,
> }
>
> /**
> + * regs_get_kernel_stack_nth_addr() - get the address of the Nth entry on stack
> + * @regs: pt_regs which contains kernel stack pointer.
> + * @n: stack entry number.
> + *
> + * regs_get_kernel_stack_nth() returns the address of the @n th entry of the
> + * kernel stack which is specified by @regs. If the @n th entry is NOT in
> + * the kernel stack, this returns NULL.
> + */
> +static inline unsigned long *regs_get_kernel_stack_nth_addr(struct pt_regs *regs,
> + unsigned int n)
> +{
> + unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
> +
> + addr += n;
> + if (regs_within_kernel_stack(regs, (unsigned long)addr))
> + return addr;
> + else
> + return NULL;
> +}
> +
> +/* To avoid include hell, we can't include uaccess.h */
> +extern long probe_kernel_read(void *dst, const void *src, size_t size);
> +
> +/**
> * regs_get_kernel_stack_nth() - get Nth entry of the stack
> * @regs: pt_regs which contains kernel stack pointer.
> * @n: stack entry number.
> *
> * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
> - * is specified by @regs. If the @n th entry is NOT in the kernel stack,
> + * is specified by @regs. If the @n th entry is NOT in the kernel stack
> * this returns 0.
> */
> static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
> unsigned int n)
> {
> - unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
> - addr += n;
> - if (regs_within_kernel_stack(regs, (unsigned long)addr))
> - return *addr;
> - else
> - return 0;
> + unsigned long *addr;
> + unsigned long val;
> + long ret;
> +
> + addr = regs_get_kernel_stack_nth_addr(regs, n);
> + if (addr) {
> + ret = probe_kernel_read(&val, addr, sizeof(val));
> + if (!ret)
> + return val;
> + }
> + return 0;
> }
>
> /**
> --
> 2.13.6
>
On Wed, 17 Oct 2018 16:59:51 -0400
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (VMware)" <[email protected]>
>
> Andy had some concerns about using regs_get_kernel_stack_nth() in a new
> function regs_get_kernel_argument() as if there's any error in the stack
> code, it could cause a bad memory access. To be on the safe side, call
> probe_kernel_read() on the stack address to be extra careful in accessing
> the memory. A helper function, regs_get_kernel_stack_nth_addr(), was added
> to just return the stack address (or NULL if not on the stack), that will be
> used to find the address (and could be used by other functions) and read the
> address with kernel_probe_read().
>
> Link: http://lkml.kernel.org/r/CALCETrXn9zKTb9i1LP3qoFcpqZHF34BdkuZ5D3N0uCmRr+VnbA@mail.gmail.com
> Requested-by: Andy Lutomirski <[email protected]>
> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
Looks good to me.
Reviewed-by: Masami Hiramatsu <[email protected]>
Thank you,
> ---
>
> Changes since v1:
>
> - Make regs_get_kernel_stack_nth() not fault, and not have a
> separate function. Only tracing uses it anyway.
>
> arch/x86/include/asm/ptrace.h | 43 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index c2304b25e2fd..055f632ce711 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -237,23 +237,52 @@ static inline int regs_within_kernel_stack(struct pt_regs *regs,
> }
>
> /**
> + * regs_get_kernel_stack_nth_addr() - get the address of the Nth entry on stack
> + * @regs: pt_regs which contains kernel stack pointer.
> + * @n: stack entry number.
> + *
> + * regs_get_kernel_stack_nth() returns the address of the @n th entry of the
> + * kernel stack which is specified by @regs. If the @n th entry is NOT in
> + * the kernel stack, this returns NULL.
> + */
> +static inline unsigned long *regs_get_kernel_stack_nth_addr(struct pt_regs *regs,
> + unsigned int n)
> +{
> + unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
> +
> + addr += n;
> + if (regs_within_kernel_stack(regs, (unsigned long)addr))
> + return addr;
> + else
> + return NULL;
> +}
> +
> +/* To avoid include hell, we can't include uaccess.h */
> +extern long probe_kernel_read(void *dst, const void *src, size_t size);
> +
> +/**
> * regs_get_kernel_stack_nth() - get Nth entry of the stack
> * @regs: pt_regs which contains kernel stack pointer.
> * @n: stack entry number.
> *
> * regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
> - * is specified by @regs. If the @n th entry is NOT in the kernel stack,
> + * is specified by @regs. If the @n th entry is NOT in the kernel stack
> * this returns 0.
> */
> static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
> unsigned int n)
> {
> - unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
> - addr += n;
> - if (regs_within_kernel_stack(regs, (unsigned long)addr))
> - return *addr;
> - else
> - return 0;
> + unsigned long *addr;
> + unsigned long val;
> + long ret;
> +
> + addr = regs_get_kernel_stack_nth_addr(regs, n);
> + if (addr) {
> + ret = probe_kernel_read(&val, addr, sizeof(val));
> + if (!ret)
> + return val;
> + }
> + return 0;
> }
>
> /**
> --
> 2.13.6
>
--
Masami Hiramatsu <[email protected]>
On Thu, 18 Oct 2018 15:48:46 +0900
Masami Hiramatsu <[email protected]> wrote:
> On Wed, 17 Oct 2018 16:59:51 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (VMware)" <[email protected]>
> >
> > Andy had some concerns about using regs_get_kernel_stack_nth() in a new
> > function regs_get_kernel_argument() as if there's any error in the stack
> > code, it could cause a bad memory access. To be on the safe side, call
> > probe_kernel_read() on the stack address to be extra careful in accessing
> > the memory. A helper function, regs_get_kernel_stack_nth_addr(), was added
> > to just return the stack address (or NULL if not on the stack), that will be
> > used to find the address (and could be used by other functions) and read the
> > address with kernel_probe_read().
> >
> > Link: http://lkml.kernel.org/r/CALCETrXn9zKTb9i1LP3qoFcpqZHF34BdkuZ5D3N0uCmRr+VnbA@mail.gmail.com
> > Requested-by: Andy Lutomirski <[email protected]>
> > Signed-off-by: Steven Rostedt (VMware) <[email protected]>
>
> Looks good to me.
>
> Reviewed-by: Masami Hiramatsu <[email protected]>
>
> Thank you,
>
Thanks Masami,
I plan on posting all the patches later today. They already passed all
my tests :-) Well, it hasn't broken anything, as I haven't added tests
to test your code yet.
-- Steve
Commit-ID: c2712b858187f5bcd7b042fe4daa3ba3a12635c0
Gitweb: https://git.kernel.org/tip/c2712b858187f5bcd7b042fe4daa3ba3a12635c0
Author: Steven Rostedt (VMware) <[email protected]>
AuthorDate: Wed, 17 Oct 2018 16:59:51 -0400
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 18 Oct 2018 08:28:35 +0200
kprobes, x86/ptrace.h: Make regs_get_kernel_stack_nth() not fault on bad stack
Andy had some concerns about using regs_get_kernel_stack_nth() in a new
function regs_get_kernel_argument() as if there's any error in the stack
code, it could cause a bad memory access. To be on the safe side, call
probe_kernel_read() on the stack address to be extra careful in accessing
the memory. A helper function, regs_get_kernel_stack_nth_addr(), was added
to just return the stack address (or NULL if not on the stack), that will be
used to find the address (and could be used by other functions) and read the
address with kernel_probe_read().
Requested-by: Andy Lutomirski <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
Reviewed-by: Joel Fernandes (Google) <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/ptrace.h | 42 +++++++++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 6de1fd3d0097..ee696efec99f 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -236,24 +236,52 @@ static inline int regs_within_kernel_stack(struct pt_regs *regs,
(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1)));
}
+/**
+ * regs_get_kernel_stack_nth_addr() - get the address of the Nth entry on stack
+ * @regs: pt_regs which contains kernel stack pointer.
+ * @n: stack entry number.
+ *
+ * regs_get_kernel_stack_nth() returns the address of the @n th entry of the
+ * kernel stack which is specified by @regs. If the @n th entry is NOT in
+ * the kernel stack, this returns NULL.
+ */
+static inline unsigned long *regs_get_kernel_stack_nth_addr(struct pt_regs *regs, unsigned int n)
+{
+ unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
+
+ addr += n;
+ if (regs_within_kernel_stack(regs, (unsigned long)addr))
+ return addr;
+ else
+ return NULL;
+}
+
+/* To avoid include hell, we can't include uaccess.h */
+extern long probe_kernel_read(void *dst, const void *src, size_t size);
+
/**
* regs_get_kernel_stack_nth() - get Nth entry of the stack
* @regs: pt_regs which contains kernel stack pointer.
* @n: stack entry number.
*
* regs_get_kernel_stack_nth() returns @n th entry of the kernel stack which
- * is specified by @regs. If the @n th entry is NOT in the kernel stack,
+ * is specified by @regs. If the @n th entry is NOT in the kernel stack
* this returns 0.
*/
static inline unsigned long regs_get_kernel_stack_nth(struct pt_regs *regs,
unsigned int n)
{
- unsigned long *addr = (unsigned long *)kernel_stack_pointer(regs);
- addr += n;
- if (regs_within_kernel_stack(regs, (unsigned long)addr))
- return *addr;
- else
- return 0;
+ unsigned long *addr;
+ unsigned long val;
+ long ret;
+
+ addr = regs_get_kernel_stack_nth_addr(regs, n);
+ if (addr) {
+ ret = probe_kernel_read(&val, addr, sizeof(val));
+ if (!ret)
+ return val;
+ }
+ return 0;
}
#define arch_has_single_step() (1)