Re: [RFC][PATCH] Prevent overriding of Symbols in the Kernel, avoiding Undefined behaviour

[Jesper Juhl <jesper.juhl@gmail.com>]

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/