Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 3 Apr 2020 13:54:28 -0400
From:   Joe Lawrence <joe.lawrence@redhat.com>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>,
        Nicolai Stange <nstange@suse.de>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [POC 04/23] livepatch: Prevent loading livepatch sub-module
 unintentionally.
Message-ID: <20200403175428.GA30284@redhat.com>
References: <20200117150323.21801-1-pmladek@suse.com>
 <20200117150323.21801-5-pmladek@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200117150323.21801-5-pmladek@suse.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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