From: Borislav Petkov <bp@alien8.de>
To: Andy Lutomirski <luto@kernel.org>
Cc: x86@kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/ptrace: Clean up PTRACE_GETREGS/PTRACE_PUTREGS regset selection
Date: Tue, 2 Feb 2021 12:32:56 +0100 [thread overview]
Message-ID: <20210202113256.GC18075@zn.tnic> (raw)
In-Reply-To: <9268050ac1fb3db6b4ec20d3ef696cc44fa3e9d0.1611884439.git.luto@kernel.org>
On Thu, Jan 28, 2021 at 05:41:21PM -0800, Andy Lutomirski wrote:
> task_user_regset_view() is fundamentally broken, but it's ABI for
> PTRACE_GETREGSET and PTRACE_SETREGSET.
>
> We shouldn't be using it for PTRACE_GETREGS or PTRACE_SETREGS,
No "We" etc pls.
> though. A native 64-bit ptrace() call and an x32 ptrace() call
> should use the 64-bit regset views, and a 32-bit ptrace() call
> (native or compat) should use the 32-bit regset.
> task_user_regset_view() almost does this except that it will
> malfunction if a ptracer is itself ptraced and the outer ptracer
> modifies CS on entry to a ptrace() syscall.
Is that the reason why task_user_regset_view() is fundamentally broken?
It is somewhat unclear what exactly is broken.
> Hopefully that has
> never happened. (The compat ptrace() code already hardcoded the
> 32-bit regset, so this patch has no effect on that path.)
>
> Fix it and deobfuscate the code by hardcoding the 64-bit view in the
> x32 ptrace() and selecting the view based on the kernel config in
> the native ptrace().
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> Every time I look at ptrace, it grosses me out. This makes it slightly
> more comprehensible.
>
> arch/x86/kernel/ptrace.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
Well, did you run the gdb testsuite on this and a bunch of other tests
we have?
I don't want us to break gdb or something else using ptrace() in some
sublte manner and then waste a bunch of time and energy chasing it, like
the DR6 thing earlier this week.
> +/*
> + * This is used by PTRACE_GETREGSET and PTRACE_SETREGSET to decide which
> + * regset format to use based on the register state of the tracee.
> + * This makes no sense whatsoever, but there appears to be existing user
> + * code that relies on it.
... because? It should use the native regset with which the kernel is
built? Please explain yourself Lutomirski!
:-)))
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
prev parent reply other threads:[~2021-02-02 11:35 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-29 1:41 [PATCH] x86/ptrace: Clean up PTRACE_GETREGS/PTRACE_PUTREGS regset selection Andy Lutomirski
2021-02-02 11:32 ` Borislav Petkov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210202113256.GC18075@zn.tnic \
--to=bp@alien8.de \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.