linux-kernel - Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

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] [day] [month] [year] [list]

Message-ID: <20170830144844.tfcpopvyidcufj7s@treble>
Date:   Wed, 30 Aug 2017 09:48:44 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Joe Lawrence <joe.lawrence@...hat.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Petr Mladek <pmladek@...e.com>,
        Chris J Arges <chris.j.arges@...onical.com>
Subject: Re: [PATCH v4 1/3] livepatch: add (un)patch callbacks

On Fri, Aug 25, 2017 at 03:10:00PM -0400, Joe Lawrence wrote:
> @@ -871,6 +882,13 @@ int klp_module_coming(struct module *mod)
>  			pr_notice("applying patch '%s' to loading module '%s'\n",
>  				  patch->mod->name, obj->mod->name);
>  
> +			ret = klp_pre_patch_callback(obj);
> +			if (ret) {
> +				pr_warn("pre-patch callback failed for object '%s'\n",
> +					obj->name);
> +				goto err;
> +			}
> +
>  			ret = klp_patch_object(obj);
>  			if (ret) {
>  				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",

If klp_pre_patch_callback() succeeds but klp_patch_object() fails, the
post-unpatch callback needs to be called in the error path.

> +/**
> + * klp_pre_patch_callback - executed before klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Return: status from callback
> + *
> + * Callers should ensure obj->patched is *not* set.

Can this comment be removed since it no longer checks obj->patched?

> +static inline int klp_pre_patch_callback(struct klp_object *obj)
> +{
> +	obj->pre_patch_callback_status =
> +		(obj->callbacks.pre_patch) ?
> +			(*obj->callbacks.pre_patch)(obj) : 0;
> +
> +	return obj->pre_patch_callback_status;
> +}
> +
> +/**
> + * klp_post_patch_callback() - executed after klp_object is patched
> + * @obj:	invoke callback for this klp_object
> + *
> + * Callers should ensure obj->patched is set.

Ditto here and below.

> +static inline void klp_post_patch_callback(struct klp_object *obj)
> +{
> +	if (obj->callbacks.post_patch)
> +		(*obj->callbacks.post_patch)(obj);
> +}
> +
> +/**
> + * klp_pre_unpatch_callback() - executed before klp_object is unpatched
> + *                              and is active across all tasks
> + * @obj:	invoke callback for this klp_object
> + *
> + * This callback will not be run if the pre-patch callback status was
> + * non-zero.

I think this comment should be a little broader.  The callback won't be
called if the object fails to patch for *any* reason.

Also, I think all the comments about when the callbacks are run or not
run would be better placed in a paragraph above the 'struct
klp_callbacks' definition in include/linux/livepatch.h.

And actually, IMO, all the functions in core.h are straightforward
enough that they don't need *any* function header comments.  And the
same for klp_is_object_loaded().  I would just remove all the core.h
comments, and just add a short explanation in livepatch.h about when the
callbacks are called and not called.

> + * Callers should ensure obj->patched is set.
> + */
> +static inline void klp_pre_unpatch_callback(struct klp_object *obj)
> +{
> +	if (!obj->pre_patch_callback_status &&
> +	    obj->callbacks.pre_unpatch)
> +		(*obj->callbacks.pre_unpatch)(obj);
> +}

I think pre_patch_callback_status (or callbacks_disabled) doesn't need
to be checked here, right?  Since if we got this far, the patching was
successful and callbacks will be enabled by definition?

-- 
Josh