linux-kernel - Re: [POC 0/7] livepatch: Make livepatch states, callbacks, and shadow variables work together

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

Message-ID: <ZsYHn3XoJu3npb0E@pathway.suse.cz>
Date: Wed, 21 Aug 2024 17:28:31 +0200
From: Petr Mladek <pmladek@...e.com>
To: Nicolai Stange <nstange@...e.de>
Cc: Miroslav Benes <mbenes@...e.cz>, Josh Poimboeuf <jpoimboe@...nel.org>,
	Joe Lawrence <joe.lawrence@...hat.com>,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [POC 0/7] livepatch: Make livepatch states, callbacks, and
 shadow variables work together

On Thu 2024-07-25 16:40:20, Nicolai Stange wrote:
> Miroslav Benes <mbenes@...e.cz> writes:
> 
> >
> > Do we still need klp_state->data member? Now that it can be easily coupled 
> > with shadow variables, is there a reason to preserve it?

Good point. I have actually forgot the pointer completely.

> I would say yes, it could point to e.g. some lock protecting an
> associated shadow variable's usage. Or be used to conveniently pass on
> any kind of data between subsequent livepatches.

Honestly, I would prefer to remove the klp_state->data member. I can't
find a safe way how to maintain it.

The pointer is in struct klp_state. It means that any livepatch
supporting the state has its own copy of the pointer. We could copy
the value in __klp_enable_patch() but where?
Before calling klp_states_pre_patch() or after?

It might somehow work when the value is set only once (in a pre_patch
callback). But what if anyone tries to modify the pointer.
What if there are more livepatches using the state at the same time
(not using the atomic replace) and each livepatch has different
value.

>From my POV, the klp_state->data pointer is a potentially dangerous
thing with not well defined semantic. IMHO, it is not worth it.

It the livepatches need a new synchronization mechanism, they
might allocate the lock in yet another shadow variable.

Do I miss anything, please?
Could you come up with a reasonable semantic, please?

Best Regards,
Petr