Re: [POC 04/23] livepatch: Prevent loading livepatch sub-module unintentionally.

[Joe Lawrence <joe.lawrence@redhat.com>]

On Fri, Jan 17, 2020 at 04:03:04PM +0100, Petr Mladek wrote:
> Livepatch is split into several modules. The main module is for livepatching
> vmlinux. The rest is for livepatching other modules.
> 
> Only the livepatch module for vmlinux can be loaded by users. Others are
> loaded automatically when the related module is or gets loaded.
> 
> Users might try to load any livepatch module. It must be allowed
> only when the related livepatch module for vmlinux and the livepatched
> module are loaded.
> 
> Also it is important to check that obj->name is listed in patch->obj_names.
> Otherwise this module would not be loaded automatically. And it would
> lead into inconsistent behavier. Anyway, the missing name means a mistake
> somewhere and must be reported.
> 
> klp_add_object() is taking over the job done by klp_module_coming().
> The error message is taken from there so that selftests do not need
> to get updated.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/livepatch/core.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index ec7ffc7db3a7..e2c7dc6c2d5f 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> 
> [ ... snip ... ]
> 
>  int klp_add_object(struct klp_object *obj)
>  {
> +	struct klp_patch *patch;
>  	int ret;
>  
>  	ret = klp_check_object(obj, true);
>  	if (ret)
>  		return ret;
>  
> +	mutex_lock(&klp_mutex);
> +
> +	patch = klp_find_patch(obj->patch_name);
> +	if (!patch) {
> +		pr_err("Can't load livepatch (%s) for module when the livepatch (%s) for vmcore is not loaded\n",
> +		       obj->mod->name, obj->patch_name);

nit: s/vmcore/vmlinux in the error message?

> +		ret = -EINVAL;
> +		goto err;

Minor code snafu: !patch for this exit path means ...

> +	}
> +
> +	if (!klp_is_object_compatible(patch, obj)) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	mutex_unlock(&klp_mutex);
>  	return 0;
> +
> +err:
> +	/*
> +	 * If a patch is unsuccessfully applied, return
> +	 * error to the module loader.
> +	 */
> +	pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n",
> +		patch->obj->patch_name, obj->name, obj->name);

... we can't access patch->obj->patch_name here.

-- Joe