linux-kernel - Re: PATCH v6 6/6] livepatch: Add atomic replace

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]

Date:   Thu, 1 Feb 2018 14:49:24 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Petr Mladek <pmladek@...e.com>
cc:     jpoimboe@...hat.com, jikos@...nel.org,
        Jason Baron <jbaron@...mai.com>, jeyu@...nel.org,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: PATCH v6 6/6] livepatch: Add atomic replace


> -struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
> +static struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
>  						struct klp_object *old_obj)

A nit, but this change belongs to 3/6, doesn't it?

>  {
>  	struct klp_object *obj;
> @@ -708,6 +764,102 @@ struct klp_object *klp_get_or_add_object(struct klp_patch *patch,
>  	return obj;
>  }

[...]

> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index 6917100fbe79..3f6cf5b9e048 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -87,6 +87,33 @@ static void klp_complete_transition(void)
>  		 klp_transition_patch->mod->name,
>  		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
>  
> +	/*
> +	 * For replace patches, we disable all previous patches, and replace
> +	 * the dynamic no-op functions by removing the ftrace hook.
> +	 */
> +	if (klp_transition_patch->replace && klp_target_state == KLP_PATCHED) {
> +		/*
> +		 * Make sure klp_ftrace_handler() can no longer see functions
> +		 * from the replaced patches.  Then all functions will be
> +		 * redirected using klp_transition_patch.  They will use
> +		 * either a new code or they will stay in the original code
> +		 * because of the nop funcs' structures.
> +		 */
> +		if (klp_forced)
> +			klp_synchronize_transition();

I'm not sure why this is here. klp_forced should not be so special that it 
would warrant a synchronization. However, I'm also not sure that it would 
be safe to remove it from the patch.

Is this because "force" is the only one who can clear TIF_PATCH_PENDING of 
other tasks? So, there could be a task running in klp_ftrace_handler(), 
its TIF_PATCH_PENDING is cleared by force and we could have a problem 
because of that since we're throwing away replaced patches.

Ok, that sounds viable. I'd welcome a comment on that in the code.

Thanks,
Miroslav

> +
> +		klp_throw_away_replaced_patches(klp_transition_patch,
> +						klp_forced);
> +
> +		/*
> +		 * There is no need to synchronize the transition after removing
> +		 * nops. They must be the last on the func_stack. Ftrace
> +		 * gurantees that nobody will stay in the trampoline after
> +		 * the ftrace handler is unregistered.
> +		 */
> +		klp_unpatch_objects(klp_transition_patch, KLP_FUNC_NOP);
> +	}
> +
>  	if (klp_target_state == KLP_UNPATCHED) {
>  		/*
>  		 * All tasks have transitioned to KLP_UNPATCHED so we can now