Re: livepatch: reuse module loader code to write relocations

[Jessica Yu]

+++ Miroslav Benes [13/01/16 10:19 +0100]:
> On Fri, 8 Jan 2016, Jessica Yu wrote:
>
> >  static int klp_write_object_relocations(struct module *pmod,
> >  					struct klp_object *obj)
> >  {
> > -	int ret = 0;
> > -	unsigned long val;
> > -	struct klp_reloc *reloc;
> > +	int i, len, ret = 0;
> > +	char *secname;
> > +	const char *objname;
> >
> >  	if (WARN_ON(!klp_is_object_loaded(obj)))
> >  		return -EINVAL;
> >
> > -	if (WARN_ON(!obj->relocs))
> > -		return -EINVAL;
> > +	objname = klp_is_module(obj) ? obj->name : "vmlinux";
> >
> >  	module_disable_ro(pmod);
> > +	/* For each klp rela section for this object */
> > +	for (i = 1; i < pmod->info->hdr->e_shnum; i++) {
> > +		if (!(pmod->info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH))
> > +			continue;
>
> One more thing. If the module does not specify it is a live patch module
> in modinfo (with MODULE_INFO(livepatch, "Y")), but it is a perfect live
> patch module otherwise (it calls klp_register_patch in its init function),
> the kernel crashes here. pmod->info is not initialized at all. This should
> be fixed. Perhaps the easiest would be to call
> klp_write_object_relocations() in klp_init_object_loaded() only if
> is_livepatch_module() returns true. Similar to a check for obj->relocs
> before.

Hm yes, that's a problem. To remedy this, I think it makes sense to
require all livepatch modules to identify themselves with the modinfo
attribute, since it is a very simple requirement. If some module calls
klp_register_patch() and it does not have the livepatch attribute,
klp_register_patch() can just return an error. We can call
is_livepatch_module() at the beginning of klp_register_patch(), and
proceed only if the check succeeds, since we'll then know that the
required structures have been properly initialized in the module
loader. What do you think?

Jessica