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

Joe Lawrence
2020-04-03T17:54:37Z

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