linux-kernel - Re: [PATCH v3] livepatch/module: Correctly handle coming and going modules

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:	Tue, 10 Mar 2015 17:58:02 +0100
From:	Petr Mladek <pmladek@...e.cz>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Miroslav Benes <mbenes@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	mingo@...nel.org, mathieu.desnoyers@...icios.com, oleg@...hat.com,
	paulmck@...ux.vnet.ibm.com, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, andi@...stfloor.org,
	rostedt@...dmis.org, tglx@...utronix.de
Subject: Re: [PATCH v3] livepatch/module: Correctly handle coming and going
 modules

On Tue 2015-03-10 09:47:01, Josh Poimboeuf wrote:
> On Tue, Mar 10, 2015 at 03:36:17PM +0100, Petr Mladek wrote:
> > On Tue 2015-03-10 09:22:04, Josh Poimboeuf wrote:
> > > On Tue, Mar 10, 2015 at 01:01:07PM +0100, Petr Mladek wrote:
> > > > On Mon 2015-03-09 09:40:55, Josh Poimboeuf wrote:
> > > > > On Mon, Mar 09, 2015 at 02:25:28PM +0100, Petr Mladek wrote:
> > > > > > diff --git a/kernel/module.c b/kernel/module.c
> > > > > > index d856e96a3cce..b3ffc231ce0d 100644
> > > > > > --- a/kernel/module.c
> > > > > > +++ b/kernel/module.c
> > > > > > @@ -3271,6 +3271,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > > > > >  	}
> > > > > >  #endif
> > > > > >  
> > > > > > +#ifdef CONFIG_LIVEPATCH
> > > > > > +	mod->klp_alive = false;
> > > > > > +#endif
> > > > > > +
> > > > > 
> > > > > I don't think you need this initialization.  It looks like the module
> > > > > struct is embedded in the mod->module_core region which is initialized
> > > > > to zero in move_module().
> > > > 
> > > > I have looked at this before but I was not able to find a code
> > > > zeroing struct module. If I get it correctly, mod->module_core
> > > > is a location where symbol table sections are copied or so.
> > > 
> > > Yeah, it's far from obvious.  AFAICT, it's cleared by the
> > > "memset(ptr, 0, mod->core_size)" line.
> > 
> > What I wanted to say is that module_core is not struct module. It
> > seems that it points to the module code. See within_module_core() and
> > how it is used().
> > 
> > By other words, I think that struct module is not zeroed and we need
> > to initialize the bool.
> > 
> > Or did I miss anything?
> 
> My understanding is that module_core is not only code.  It also contains the
> struct module itself.  Verified in crash:
> 
>   crash> mod |head -n2
>        MODULE       NAME                        SIZE  OBJECT FILE
>   ffffffffa0003180  video                      19905  (not loaded)  [CONFIG_KALLSYMS]
>   crash> module.module_core,core_size 0xffffffffa0003180
>     module_core = 0xffffffffa0000000
>     core_size = 0x4dc1

OK, you are right that struct module is inside mod->module_core. But I
am still not convinced that the structure is zeroed.

There are the following commands in move_module()

	ptr = module_alloc_update_bounds(mod->core_size);
[...]
	memset(ptr, 0, mod->core_size);
	mod->module_core = ptr;

	if (mod->init_size) {
[...]
	} else
		mod->module_init = NULL;


The needed memory is allocated and zeroed but the pointer
is written to the temporary place.

I do not see any code that would copy parts of struct module from the
temporary place to the newly allocated one. It seems that the whole
structure is copied.

Huh, the code is really twisted but I think that the space for the
temporary structure is not zeroed. One week proof is that the code
does mod->module_init = NULL; It would not make sense if the
temporary location was zeroed.


Of course, I will be happy if anyone convince me that I am wrong and
we could omit the initialization.

Best Regards,
Petr
--
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/