Message-ID: <20170325014140.GB1251@athena>
Date:   Fri, 24 Mar 2017 19:41:40 -0600
From:   Eddie Kovsky <ewk@...ovsky.org>
To:     Jessica Yu <jeyu@...hat.com>
Cc:     rusty@...tcorp.com.au, keescook@...omium.org,
        linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v3 1/2] module: verify address is read-only

On 03/24/17, Jessica Yu wrote:
> +++ Eddie Kovsky [22/03/17 20:55 -0600]:
> > Implement a mechanism to check if a module's address is in
> > the rodata or ro_after_init sections. It mimics the exsiting functions
> > that test if an address is inside a module's text section.
> > 
> > Functions that take a module as an argument will be able to
> 
> > verify that the module is in a read-only section.
> 
> s/module/module address/?
> 
Yes, that is more accurate.

> Also, there is some useful information in your cover letter on the
> context and motivation for adding to the api, it would be good to
> reproduce that info here so that we can have it officially in the
> changelog. (sentences "This implements a suggestion made by Kees..."
> and "The idea is to prevent structures..." would be nice to copy here)
> 
Okay, that's easy to add.

Kees, would you like me to add your Suggested-by as well?


> > Signed-off-by: Eddie Kovsky <ewk@...ovsky.org>
> > ---
> > Changes in v3:
> > - Change function name is_module_ro_address() to
> > is_module_rodata_address().
> > - Improve comments on is_module_rodata_address().
> > - Add a !CONFIG_MODULES stub for is_module_rodata_address.
> > - Correct and simplify the check for the read-only memory regions in
> > the module address.
> > 
> > include/linux/module.h | 12 ++++++++++++
> > kernel/module.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+)
> > 
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 9ad68561d8c2..66b7fd321334 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -492,7 +492,9 @@ static inline int module_is_live(struct module *mod)
> > 
> > struct module *__module_text_address(unsigned long addr);
> > struct module *__module_address(unsigned long addr);
> > +struct module *__module_ro_address(unsigned long addr);
> 
> Can we rename __module_ro_address to __module_rodata_address (to match
> is_module_rodata_address())?
> 
Sure, consistency is a good thing.

> > bool is_module_address(unsigned long addr);
> > +bool is_module_rodata_address(unsigned long addr);
> > bool __is_module_percpu_address(unsigned long addr, unsigned long *can_addr);
> > bool is_module_percpu_address(unsigned long addr);
> > bool is_module_text_address(unsigned long addr);
> > @@ -646,6 +648,16 @@ static inline struct module *__module_address(unsigned long addr)
> > 	return NULL;
> > }
> > 
> > +static inline struct module *__module_ro_address(unsigned long addr)
> > +{
> > +	return NULL;
> > +}
> > +
> > +static inline bool is_module_rodata_address(unsigned long addr)
> > +{
> > +	return false;
> > +}
> > +
> 
> Style nitpick: Can you move is_module_rodata_address() below is_module_address()?
> That way, the function stubs are grouped a bit more consistently.
> 
I think I might have shuffled that on the last rebase.

> > static inline struct module *__module_text_address(unsigned long addr)
> > {
> > 	return NULL;
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 8ffcd29a4245..99ada1257aaa 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -4292,6 +4292,59 @@ struct module *__module_text_address(unsigned long addr)
> > }
> > EXPORT_SYMBOL_GPL(__module_text_address);
> > 
> > +/**
> > + * is_module_rodata_address - is this address inside read-only module code?
> 
> s/code/data/
> 
Yes.

> > + * @addr: the address to check.
> > + *
> > + */
> > +bool is_module_rodata_address(unsigned long addr)
> > +{
> > +	bool ret;
> > +
> > +	preempt_disable();
> > +	ret = __module_ro_address(addr) != NULL;
> > +	preempt_enable();
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * __module_ro_address - get the module whose rodata/ro_after_init sections
> > + * contain the given address.
> 
> As mentioned in the first comment, let's rename __module_ro_address to
> __module_rodata_address, so it mirrors is_module_rodata_address().
> 
> > + * @addr: the address.
> > + *
> > + * Must be called with preempt disabled or module mutex held so that
> > + * module doesn't get freed during this.
> > + */
> > +struct module *__module_ro_address(unsigned long addr)
> > +{
> > +	struct module *mod = __module_address(addr);
> 
> We need to check that mod is not NULL before dereferencing it below.
> 
I missed that. The new variables are now using mod before we check it.
I'll fix it on the next round.

> > +	void *init_base = mod->init_layout.base;
> > +	unsigned int init_text_size = mod->init_layout.text_size;
> > +	unsigned int init_ro_after_init_size = mod->init_layout.ro_after_init_size;
> > +
> > +	void *core_base = mod->core_layout.base;
> > +	unsigned int core_text_size = mod->core_layout.text_size;
> > +	unsigned int core_ro_after_init_size = mod->core_layout.ro_after_init_size;
> > +
> > +	/*
> > +	 * Make sure module is within the read-only section.
> > +	 * range(base + text_size, base + ro_after_init_size)
> > +	 * encompasses both the rodata and ro_after_init regions.
> > +	 * See comment above frob_text() for the layout diagram.
> > +	 */
> > +	if (mod) {
> > +		if (!within(addr, init_base + init_text_size,
> > +			    init_ro_after_init_size - init_text_size)
> > +		    && !within(addr, core_base + core_text_size,
> > +			       core_ro_after_init_size - core_text_size))
> > +			mod = NULL;
> > +	}
> > +	return mod;
> > +}
> > +EXPORT_SYMBOL_GPL(__module_ro_address);
> > +
> > /* Don't grab lock, we're oopsing. */
> > void print_modules(void)
> > {
> > --
> > 2.12.0

Powered by blists - more mailing lists