From: Masami Hiramatsu <[email protected]>
Impact: fix ref-after-free crash on failed module load
Fix refptr bug: Change refptr allocation and release order not to access a module
data structure pointed by 'mod' after freeing mod->module_core.
This bug will cause kernel panic(e.g. failed to find undefined symbols).
This bug was reported on systemtap bugzilla.
http://sources.redhat.com/bugzilla/show_bug.cgi?id=9927
Signed-off-by: Masami Hiramatsu <[email protected]>
Cc: Eric Dumazet <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/kernel/module.c b/kernel/module.c
index ba22484..1196f5d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2015,14 +2015,6 @@ static noinline struct module *load_module(void __user *umod,
if (err < 0)
goto free_mod;
-#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
- mod->refptr = percpu_modalloc(sizeof(local_t), __alignof__(local_t),
- mod->name);
- if (!mod->refptr) {
- err = -ENOMEM;
- goto free_mod;
- }
-#endif
if (pcpuindex) {
/* We have a special allocation for this section. */
percpu = percpu_modalloc(sechdrs[pcpuindex].sh_size,
@@ -2030,7 +2022,7 @@ static noinline struct module *load_module(void __user *umod,
mod->name);
if (!percpu) {
err = -ENOMEM;
- goto free_percpu;
+ goto free_mod;
}
sechdrs[pcpuindex].sh_flags &= ~(unsigned long)SHF_ALLOC;
mod->percpu = percpu;
@@ -2082,6 +2074,14 @@ static noinline struct module *load_module(void __user *umod,
/* Module has been moved. */
mod = (void *)sechdrs[modindex].sh_addr;
+#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
+ mod->refptr = percpu_modalloc(sizeof(local_t), __alignof__(local_t),
+ mod->name);
+ if (!mod->refptr) {
+ err = -ENOMEM;
+ goto free_init;
+ }
+#endif
/* Now we've moved module, initialize linked lists, etc. */
module_unload_init(mod);
@@ -2288,15 +2288,17 @@ static noinline struct module *load_module(void __user *umod,
ftrace_release(mod->module_core, mod->core_size);
free_unload:
module_unload_free(mod);
+ free_init:
+#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
+ percpu_modfree(mod->refptr);
+#endif
module_free(mod, mod->module_init);
free_core:
module_free(mod, mod->module_core);
+ /* mod will be freed with core. Don't access it beyond this line! */
free_percpu:
if (percpu)
percpu_modfree(percpu);
-#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
- percpu_modfree(mod->refptr);
-#endif
free_mod:
kfree(args);
free_hdr:
Rusty Russell a écrit :
> From: Masami Hiramatsu <[email protected]>
>
> Impact: fix ref-after-free crash on failed module load
>
> Fix refptr bug: Change refptr allocation and release order not to access a module
> data structure pointed by 'mod' after freeing mod->module_core.
> This bug will cause kernel panic(e.g. failed to find undefined symbols).
>
> This bug was reported on systemtap bugzilla.
> http://sources.redhat.com/bugzilla/show_bug.cgi?id=9927
>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Signed-off-by: Rusty Russell <[email protected]>
My original patch did not have this problem, because I used a local variable
to hold refptr.
http://www.archivum.info/[email protected]/2008-05/msg07400.html
A simpler patch could just use a local variable again, since we are very
late in rc phase ?
From: Masami Hiramatsu <[email protected]>
Impact: fix ref-after-free crash on failed module load
Fix refptr bug: Change refptr release not to access a module
data structure pointed by 'mod' after freeing mod->module_core.
This bug will cause kernel panic(e.g. failed to find undefined symbols).
This bug was reported on systemtap bugzilla.
http://sources.redhat.com/bugzilla/show_bug.cgi?id=9927
Signed-off-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/kernel/module.c b/kernel/module.c
index ba22484..4dd1228 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1874,6 +1874,9 @@ static noinline struct module *load_module(void __user *umod,
struct kernel_param *kp;
struct module *mod;
long err = 0;
+#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
+ void *refptr = NULL;
+#endif
void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
unsigned long *mseg;
mm_segment_t old_fs;
@@ -2018,6 +2021,7 @@ static noinline struct module *load_module(void __user *umod,
#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
mod->refptr = percpu_modalloc(sizeof(local_t), __alignof__(local_t),
mod->name);
+ refptr = mod->refptr;
if (!mod->refptr) {
err = -ENOMEM;
goto free_mod;
@@ -2292,10 +2296,14 @@ static noinline struct module *load_module(void __user *umod,
free_core:
module_free(mod, mod->module_core);
free_percpu:
+ /*
+ * Do not access to mod anymore, it's now freed
+ */
if (percpu)
percpu_modfree(percpu);
#if defined(CONFIG_MODULE_UNLOAD) && defined(CONFIG_SMP)
- percpu_modfree(mod->refptr);
+ if (refptr)
+ percpu_modfree(refptr);
#endif
free_mod:
kfree(args);
On Tuesday 17 March 2009 13:54:36 Eric Dumazet wrote:
> Rusty Russell a écrit :
> > From: Masami Hiramatsu <[email protected]>
> >
> > Impact: fix ref-after-free crash on failed module load
> >
> > Fix refptr bug: Change refptr allocation and release order not to access a module
> > data structure pointed by 'mod' after freeing mod->module_core.
> > This bug will cause kernel panic(e.g. failed to find undefined symbols).
> >
> > This bug was reported on systemtap bugzilla.
> > http://sources.redhat.com/bugzilla/show_bug.cgi?id=9927
> >
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > Cc: Eric Dumazet <[email protected]>
> > Signed-off-by: Rusty Russell <[email protected]>
>
>
> My original patch did not have this problem, because I used a local variable
> to hold refptr.
>
> http://www.archivum.info/[email protected]/2008-05/msg07400.html
>
> A simpler patch could just use a local variable again, since we are very
> late in rc phase ?
Right, it was my mistake. But the extra #ifdef is still ugly: moving the code
is a larger patch, but it's not complicated.
Thanks,
Rusty.