Re: [RFC PATCH 2/2] livepatch: Implement livepatch hybrid mode

[Petr Mladek]

On Fri 2025-02-07 11:16:45, Yafang Shao wrote:
> On Fri, Feb 7, 2025 at 10:31 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > On Mon, Jan 27, 2025 at 02:35:26PM +0800, Yafang Shao wrote:
> > > - Temporary Loss of Patching
> > >
> > >   During the replacement process, the old patch is set to a NOP (no-operation)
> > >   before the new patch is fully applied. This creates a window where the
> > >   function temporarily reverts to its original, unpatched state. If the old
> > >   patch fixed a critical issue (e.g., one that prevented a system panic), the
> > >   system could become vulnerable to that issue during the transition.
> >
> > Are you saying that atomic replace is not atomic?  If so, this sounds
> > like another bug.
> 
> >From my understanding, there’s a window where the original function is
> not patched.

This is a misunderstanding.

> klp_enable_patch
> + klp_init_patch
>    + if (patch->replace)
>           klp_add_nops(patch);  <<<< set all old patches to nop

1. The "nop" entry is added into the _new_ (to-be-enabled) livepatch,
   see klp_add_nops(patch). The parameter is the _newly_ enabled patch.

2. The "nop" entries are added only for functions which are currently
   livepatched but they are not longer livepatched in the new
   livepatch, see:

static int klp_add_object_nops(struct klp_patch *patch,
			       struct klp_object *old_obj)
{
[...]
	klp_for_each_func(old_obj, old_func) {
		func = klp_find_func(obj, old_func);
		if (func)
			continue;	<------ Do not allocate nop
						when the fuction is
						implemeted in the new
						livepatch.

		func = klp_alloc_func_nop(old_func, obj);
		if (!func)
			return -ENOMEM;
	}

	return 0;
}


> + __klp_enable_patch
>    + klp_patch_object
>       + klp_patch_func
>          + ops = klp_find_ops(func->old_func);
>             + if (ops)
>                    // add the new patch to the func_stack list
>                    list_add_rcu(&func->stack_node, &ops->func_stack);
> 
> 
> klp_ftrace_handler
> + func = list_first_or_null_rcu(&ops->func_stack, struct klp_func

3. You omitted this important part of the code:

	if (unlikely(func->transition)) {
		patch_state = current->patch_state;
		if (patch_state == KLP_TRANSITION_UNPATCHED) {
			/*
---->			 * Use the previously patched version of the function.  
---->			 * If no previous patches exist, continue with the
---->			 * original function.
			 */
			func = list_entry_rcu(func->stack_node.next,
					      struct klp_func, stack_node);


	The condition "patch_state == KLP_TRANSITION_UNPATCHED" might
	be a bit misleading.

	The state "KLP_TRANSITION_UNPATCHED" means that it can't use
	the code from the "new" livepatch => it has to fallback
	to the previously used code => previous livepatch.


> + if (func->nop)
>        goto unlock;
> + ftrace_regs_set_instruction_pointer(fregs, (unsigned long)func->new_func);

> Before the new atomic replace patch is added to the func_stack list,
> the old patch is already set to nop.
      ^^^ 
     
     The nops are set in the _new_ patch for functions which will
     not longer get livepatched, see the commit e1452b607c48c642
     ("livepatch: Add atomic replace") for more details.
     
> If klp_ftrace_handler() is
> triggered at this point, it will effectively do nothing—in other
> words, it will execute the original function.
> I might be wrong.

Fortunately, you are wrong. This would be a serious violation of
the consistency model and livepatches modifying some semantic would
blow up systems.

Best Regards,
Petr