linux-kernel - Re: [PATCH 2/5] ftrace, x86: make kernel text writable only for conversions

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:	Fri, 20 Feb 2009 12:52:33 -0500 (EST)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Ingo Molnar <mingo@...e.hu>
cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <srostedt@...hat.com>
Subject: Re: [PATCH 2/5] ftrace, x86: make kernel text writable only for
 conversions


On Fri, 20 Feb 2009, Ingo Molnar wrote:

> 
> * Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index b55b4a7..5564cf3 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -80,4 +80,14 @@ extern void return_to_handler(void);
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> >  
> > +#ifndef __ASSEMBLY__
> > +#ifdef CONFIG_DEBUG_RODATA
> > +void set_kernel_text_rw(void);
> > +void set_kernel_text_ro(void);
> > +#else
> > +static inline void set_kernel_text_rw(void) { }
> > +static inline void set_kernel_text_ro(void) { }
> > +#endif
> > +#endif /* __ASSEMBLY__ */
> 
> The proper place for this is where the other pagetable attribute 
> functions reside: arch/x86/include/asm/cacheflush.h.

Heh, I put it there because I was not sure where the proper place should 
go. I figured someone would comment and tell me where to put it ;-)

> 
> > +/* used by ftrace */
> > +void set_kernel_text_rw(void)
> > +{
> > +	unsigned long start = PFN_ALIGN(_text);
> > +	unsigned long size = PFN_ALIGN(_etext) - start;
> > +
> > +	printk(KERN_INFO "Set kernel text: %lx - %lx for read write\n",
> > +	       start, start+size);
> > +
> > +	set_pages_rw(virt_to_page(start), size >> PAGE_SHIFT);
> 
> Wont this be executed by every suspend/resume cycle? I'm not 
> sure we want a KERN_INFO printout every time.

Yeah, agreed. My original thought was to allow a sysadmin to know when
something was changing the kernel text to read/write. But it does fill
up the logs quite a bit. I'll switch it to a pr_debug.

> 
> > +/* used by ftrace */
> > +void set_kernel_text_rw(void)
> 
> i'd leave out the 'used by ftrace' bit - more uses might arise. 
> How does kprobes get around readonly pages, it uses these APIs 
> too, right?

kprobes uses text_poke. text_poke uses vmap to create its own page table 
pointers to the text memory, does the modification and then removes the 
pointers. This is quite heavy weight and since ftrace needs to modify 10s 
of thousands of areas, converting all of kernel text page tables is much 
more efficient.

Note, if kprobes did change the original page tables, then it too would 
have hit the split_large_page bug too.

-- Steve

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