linux-kernel - Re: [POC 7/7] livepatching: Remove per-state version

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: <alpine.LSU.2.21.2407251553420.21729@pobox.suse.cz>
Date: Thu, 25 Jul 2024 16:16:44 +0200 (CEST)
From: Miroslav Benes <mbenes@...e.cz>
To: Petr Mladek <pmladek@...e.com>
cc: Josh Poimboeuf <jpoimboe@...nel.org>, 
    Joe Lawrence <joe.lawrence@...hat.com>, Nicolai Stange <nstange@...e.de>, 
    live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [POC 7/7] livepatching: Remove per-state version

On Fri, 10 Nov 2023, Petr Mladek wrote:

> The livepatch state API was added to help with maintaining:
> 
>    + changes done by livepatch callbasks
>    + lifetime of shadow variables
> 
> The original API was hard to use. Both objectives are better handled
> by the new per-state callbacks. They are called when the state is
> introduced or removed. There is also support for automatically freeing
> obsolete shadow variables.
> 
> The new callbacks changed the view of compatibility.  The livepatch
> can be replaced to any older one as long the current livepatch is
> able to disable the obsolete state.
> 
> As a result, the new patch does not need to support the currently
> used states. The current patch will be able to disable them.
> 
> The remaining question is what to do with the per-state version.
> It was supposed to allow doing more modifications on an existing
> state. The experience shows that it is not needed in practice.
> 
> Well, it still might make sense to prevent downgrade when the state
> could not be disabled easily or when the author does not want to
> deal with it.
> 
> Replace the per-state version with per-state block_disable flag.
> It allows to handle several scenarios:

I have no opinion to be honest. block_disable flag might be sufficient in 
the end.

[...]

> @@ -159,7 +159,9 @@ struct klp_state {
>   * @mod:	reference to the live patch module
>   * @objs:	object entries for kernel objects to be patched
>   * @states:	system states that can get modified
> + * version:	livepatch version (optional)
>   * @replace:	replace all actively used patches
> + *
>   * @list:	list node for global list of actively used patches
>   * @kobj:	kobject for sysfs resources
>   * @obj_list:	dynamic list of the object entries
> @@ -173,6 +175,7 @@ struct klp_patch {
>  	struct module *mod;
>  	struct klp_object *objs;
>  	struct klp_state *states;
> +	unsigned int version;
>  	bool replace;

Is it still needed then? What would be the use case?

[...]

>  	/*
>  	 * Allow to reverse a pending transition in both ways. It might be
>  	 * necessary to complete the transition without forcing and breaking
> @@ -1097,10 +1104,10 @@ int klp_enable_patch(struct klp_patch *patch)
>  
>  	if (!klp_is_patch_compatible(patch)) {
>  		pr_err("Livepatch patch (%s) is not compatible with the already installed livepatches.\n",
> -			patch->mod->name);
> +		       patch->mod->name);
>  		mutex_unlock(&klp_mutex);
>  		return -EINVAL;
> -	}
> +       }
>  
>  	if (!try_module_get(patch->mod)) {
>  		mutex_unlock(&klp_mutex);
> @@ -1111,17 +1118,17 @@ int klp_enable_patch(struct klp_patch *patch)
>  
>  	ret = klp_init_patch(patch);
>  	if (ret)
> -		goto err;
> +		goto unlock_free;
>  
>  	ret = __klp_enable_patch(patch);
>  	if (ret)
> -		goto err;
> +		goto unlock_free;
>  
>  	mutex_unlock(&klp_mutex);
>  
>  	return 0;
>  
> -err:
> +unlock_free:
>  	klp_free_patch_start(patch);

Unrelated changes.
  
>  /*
>   * Check that the new livepatch will not break the existing system states.
> - * Cumulative patches must handle all already modified states.
> - * Non-cumulative patches can touch already modified states.
> + * The patch could replace existing patches only when the obsolete
> + * states can be disabled.
>   */
>  bool klp_is_patch_compatible(struct klp_patch *patch)
>  {
>  	struct klp_patch *old_patch;
>  	struct klp_state *old_state;
>  
> +	/* Non-cumulative patches are always compatible. */
> +	if (!patch->replace)
> +		return true;
> +

Cumulative != atomic replace. Those are two different things.

Miroslav