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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200403175428.GA30284@redhat.com>
Date:   Fri, 3 Apr 2020 13:54:28 -0400
From:   Joe Lawrence <joe.lawrence@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Miroslav Benes <mbenes@...e.cz>,
        Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
        Nicolai Stange <nstange@...e.de>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [POC 04/23] livepatch: Prevent loading livepatch sub-module
 unintentionally.

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@...e.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