linux-kernel - Re: [PATCH 04/11] ftrace: comment arch ftrace code

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 22 Oct 2008 12:09:51 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, fweisbec@...il.com,
	sagar.abhishek@...il.com, davem@...emloft.net, tglx@...utronix.de,
	peterz@...radead.org, torvalds@...ux-foundation.org,
	srostedt@...hat.com
Subject: Re: [PATCH 04/11] ftrace: comment arch ftrace code

On Wed, 22 Oct 2008 14:43:17 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> Add comments to explain what is happening in the x86 arch ftrace code.
> 
> Signed-off-by: Steven Rostedt <srostedt@...hat.com>
> ---
>  arch/x86/kernel/ftrace.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Index: linux-compile.git/arch/x86/kernel/ftrace.c
> ===================================================================
> --- linux-compile.git.orig/arch/x86/kernel/ftrace.c	2008-10-22 13:15:36.000000000 -0400
> +++ linux-compile.git/arch/x86/kernel/ftrace.c	2008-10-22 13:16:35.000000000 -0400
> @@ -66,18 +66,23 @@ ftrace_modify_code(unsigned long ip, uns
>  	/*
>  	 * Note: Due to modules and __init, code can
>  	 *  disappear and change, we need to protect against faulting
> -	 *  as well as code changing.
> +	 *  as well as code changing. We do this by using the
> +	 *  __copy_*_user functions.
>  	 *
>  	 * No real locking needed, this code is run through
>  	 * kstop_machine, or before SMP starts.
>  	 */
> +
> +	/* read the text we want to modify */
>  	if (__copy_from_user_inatomic(replaced, (char __user *)ip,
>  				      MCOUNT_INSN_SIZE))
>  		return -1;
>  
> +	/* Make sure it is what we expect it to be */
>  	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
>  		return -1;
>  
> +	/* replace the text with the new text */
>  	if (__copy_to_user_inatomic((char __user *)ip, new_code,
>  				    MCOUNT_INSN_SIZE))
>  		return -1;
> 

I dunno.

__copy_to_user_inatomic() is for "copying memory from userspace while
in an atomic context".

But what you're doing here is "modifying some kernel text which might
generate a fault".  It seems somewhat interface-abusive to use a
userspace access function for that just because it happens right now to
do the right thing.

I'd suggest that for clarity and for future-safety, you create some new
interface function which does that thing.  Right now it can be a simple
wrapper around __copy_from_user_inatomic().

<looks>

oh, someone added one - probe_kernel_write().  Why not use that?

<wonders why he doesn't know what's going on any more>


Also, I hope that the above code is called from within a
pagefault_disable()d region?  Or are relying upon some magical
side-effect of something which happens to do the same thing as
pagefault_disable()?  IOW: by what means does the above code ensure
that do_page_fault() will see in_atomic()==true?



--
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/