Messages in this thread Patch in this message | | | Date | Tue, 13 Dec 2005 17:49:44 +0100 | From | Jesper Juhl <> | Subject | Re: [RFC][PATCH] Prevent overriding of Symbols in the Kernel, avoiding Undefined behaviour |
| |
On 12/13/05, Ashutosh Naik <ashutosh.naik@gmail.com> wrote: > On 12/13/05, Rusty Russell <rusty@rustcorp.com.au> wrote: > > > > How about something like: > > [snip imrovement suggestion] > > Have tried that in the attached patch. However, mod->syms[i].name > would be valid only after a long relocation for loop has run through. > While this adds a wee bit extra overhead, that overhead is only in the > case where the module does actually export a Duplicate Symbol. > > This its a question, whether we do the search before relocation ( A > little messier ) or after ( More straight forward) >
hmm, patch still attached instead of inlined... :-(
Anyway, a few minor comments below.
> > --- /usr/src/linux-2.6.15-rc5-vanilla/kernel/module.c 2005-12-07 19:32:23.000000000 +0530 > +++ /usr/src/linux-2.6.15-rc5/kernel/module.c 2005-12-13 19:44:43.000000000 +0530 > @@ -1204,6 +1204,42 @@ void *__symbol_get(const char *symbol) > } > EXPORT_SYMBOL_GPL(__symbol_get); > > +/* > + * Ensure that an exported symbol [global namespace] does not already exist > + * in the Kernel or in some other modules exported symbol table. > + */ > +static int verify_export_symbols(struct module *mod) > +{ > + const char *name=0;
CodingStyle issue : const char *name = 0;
> + unsigned long i, ret = 0; > + struct module *owner; > + const unsigned long *crc; > + > + spin_lock_irq(&modlist_lock); > + for (i = 0; i < mod->num_syms; i++) > + if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc,1))) {
CodingStyle issue : if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc, 1))) {
> + name = mod->syms[i].name; > + ret = -ENOEXEC; > + goto dup; > + } > + > + for (i = 0; i < mod->num_gpl_syms; i++) > + if (unlikely(__find_symbol(mod->gpl_syms[i].name, &owner, &crc,1))) {
CodingStyle issue : if (unlikely(__find_symbol(mod->gpl_syms[i].name, &owner, &crc, 1))) {
> + name = mod->gpl_syms[i].name; > + ret = -ENOEXEC; > + goto dup; > + } > + > +dup: > + spin_unlock_irq(&modlist_lock); > + > + if (ret) > + printk("%s: exports duplicate symbol %s (owned by %s)\n",
I still think this should be printk(KERN_ERROR ...) and not just a warning, since the loading of the module will fail completely. Others may disagree ofcourse, but that's my oppinion.
> + mod->name, name, module_name(owner)); > + > + return ret; > +} > + > /* Change all symbols so that sh_value encodes the pointer directly. */ > static int simplify_symbols(Elf_Shdr *sechdrs, > unsigned int symindex, > @@ -1767,6 +1804,12 @@ static struct module *load_module(void _ > goto cleanup; > } > > + /* Find duplicate symbols */ > + err = verify_export_symbols(mod); > + > + if (err < 0) > + goto cleanup; > + > /* Set up and sort exception table */ > mod->num_exentries = sechdrs[exindex].sh_size / sizeof(*mod->extable); > mod->extable = extable = (void *)sechdrs[exindex].sh_addr; >
A few general things:
I still worry a bit about the spinlock hold time, especially since you are doing two linear searches through what could potentially be a *lot* of symbols.. It may not be a problem (do you have any time measurements?), but it still seems to me that using a lock type that allows you to sleep + a call to schedule() would be a good thing for those loops.
Also, what about the softlockup watchdog? Do we risk falsly triggering that? Would a call to touch_softlockup_watchdog() between the two loops, or maybe even inside each loop, be a good idea? again, depends on how long this all takes.
All in all, looks a lot better to me than the previous version.
-- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |