| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140114200445.6c1ce0ea@gandalf.local.home>
Date: Tue, 14 Jan 2014 20:04:45 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Petr Mladek <pmladek@...e.cz>
Cc: Frederic Weisbecker <fweisbec@...il.com>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
x86@...nel.org
Subject: Re: [PATCH v6 6/8] x86: modify ftrace function using the new
int3-based framework
On Tue, 10 Dec 2013 16:42:18 +0100
Petr Mladek <pmladek@...e.cz> wrote:
> The commit fd4363fff3d9 (x86: Introduce int3 (breakpoint)-based instruction
> patching) uses the same technique that has been used in ftrace since 08d636b
> ("ftrace/x86: Have arch x86_64 use breakpoints instead of stop machine")
>
> It would be great to use the new generic implementation and clean up the x86
> ftrace code a bit.
>
> Let's start with ftrace_modify_code. It does basically the same as text_poke_bp.
> In addition, it checks whether the replaced code is the expected one to avoid
> problems with modules. The checking code can be split from
> ftrace_modify_code_direct.
>
> Note that we do not longer need to set modifying_ftrace_code in
> ftrace_update_ftrace_func. The int3 interrupt will be handled by
> poke_int3_handler.
>
> Signed-off-by: Petr Mladek <pmladek@...e.cz>
> ---
> arch/x86/kernel/ftrace.c | 120 +++++++++++++++++++----------------------------
> 1 file changed, 47 insertions(+), 73 deletions(-)
>
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index d4bdd253fea7..5ade40e4a175 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -52,6 +52,33 @@ union ftrace_code_union {
> } __attribute__((packed));
> };
>
> +/*
> + * Note: Due to modules and __init, code can
> + * disappear and change, we need to protect against faulting
> + * as well as code changing. We do this by using the
> + * probe_kernel_* functions.
Actually the above comment is no longer accurate. I know that you just
copied it from my code, and this is my fault. But instead of keeping an
inaccurate comment around, lets fix it.
/*
* Note: Modifying kernel code is extremely critical to get right.
* We must try to detect any error, and we should only change code
* with code that we expect to be changed. Always test that the old
* text is what we expect it to be before we modify it, and fail
* if it is not.
*/
> + */
> +static int
> +ftrace_check_code(unsigned long ip, unsigned const char *expected)
> +{
> + unsigned char actual[MCOUNT_INSN_SIZE];
> +
> + if (probe_kernel_read(actual, (void *)ip, MCOUNT_INSN_SIZE))
> + return -EFAULT;
> +
> + if (memcmp(actual, expected, MCOUNT_INSN_SIZE) != 0) {
> + WARN_ONCE(1,
> + "ftrace check failed: %x %x%x%x%x vs. %x %x%x%x%x\n",
> + actual[0],
> + actual[1], actual[2], actual[3], actual[4],
> + expected[0],
> + expected[1], expected[2], expected[3], expected[4]);
> + return -EINVAL;
Hmm, I guess I'm fine with warning here like this, but it is also
covered in kernel/ftrace.c too, with the return code o -EINVAL.
But duplicate warnings may not be that bad.
> + }
> +
> + return 0;
> +}
> +
> static int ftrace_calc_offset(long ip, long addr)
> {
> return (int)(addr - ip);
> @@ -103,28 +130,12 @@ static int
> ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code,
> unsigned const char *new_code)
> {
> - unsigned char replaced[MCOUNT_INSN_SIZE];
> -
> - /*
> - * Note: Due to modules and __init, code can
> - * disappear and change, we need to protect against faulting
> - * as well as code changing. We do this by using the
> - * probe_kernel_* functions.
> - *
> - * No real locking needed, this code is run through
> - * kstop_machine, or before SMP starts.
> - */
> + int ret;
>
> - /* read the text we want to modify */
> - if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE))
> - return -EFAULT;
> -
> - /* Make sure it is what we expect it to be */
> - if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
> - return -EINVAL;
> + ret = ftrace_check_code(ip, old_code);
Wait? If this fails, why not return with ret? Otherwise, we just
returned with success!
-- Steve
>
> /* replace the text with the new text */
> - if (do_ftrace_mod_code(ip, new_code))
> + if (!ret && do_ftrace_mod_code(ip, new_code))
> return -EPERM;
>
> sync_core();
> @@ -132,6 +143,22 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code,
> return 0;
> }
>
> +static int
> +ftrace_modify_code(unsigned long ip, unsigned const char *old_code,
> + unsigned const char *new_code)
> +{
> + int ret;
> +
> + ret = ftrace_check_code(ip, old_code);
> +
> + /* replace the text with the new text */
> + if (!ret)
> + ret = text_poke_bp((void *)ip, new_code, MCOUNT_INSN_SIZE,
> + (void *)ip + MCOUNT_INSN_SIZE);
> +
> + return ret;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists