Re:

[Nicolai Stange <nstange@suse.de>]

Hi all,

Joe Lawrence <joe.lawrence@redhat.com> writes:

> On Sun, Jul 14, 2024 at 09:59:32PM +0200, raschupkin.ri@gmail.com wrote:
>> 
> But first, let me see if I understand the problem correctly.  Let's say
> points A and A' below represent the original kernel code reference
> get/put pairing in task execution flow.  A livepatch adds a new get/put
> pair, B and B' in the middle like so:
>
>   ---  execution flow  --->
>   -- A  B       B'  A'  -->
>
> There are potential issues if the livepatch is (de)activated
> mid-sequence, between the new pairings:
>
>   problem 1:
>   -- A      .   B'  A'  -->                   'B, but no B =  extra put!
>             ^ livepatch is activated here
>
>   problem 2:
>   -- A  B   .       A'  -->                   B, but no B' =  extra get!
>             ^ livepatch is deactivated here

I can confirm that this scenario happens quite often with real world CVE
fixes and there's currently no way to implement such changes safely from
a livepatch. But I also believe this is an instance of a broader problem
class we attempted to solve with that "enhanced" states API proposed and
discussed at LPC ([1], there's a link to a recording at the bottom). For
reference, see Petr's POC from [2].


> The first thing that comes to mind is that this might be solved using
> the existing shadow variable API.

Same.


> When the livepatch takes the new
> reference (B), it could create a new <struct, NEW_REF> shadow variable
> instance.  The livepatch code to return the reference (B') would then
> check on the shadow variable existence before doing so.  This would
> solve problem 1.
>
> The second problem is a little trickier.  Perhaps the shadow variable
> approach still works as long as a pre-unpatch hook* were to iterate
> through all the <*, NEW_REF> shadow variable instances and returned
> their reference before freeing the shadow variable and declaring the
> livepatch inactive.

I think the problem of consistently maintaining shadowed reference
counts (or anything shadowed for that matter) could be solved with the
help of aforementioned states API enhancements, so I would propose to
revive Petr's IMO more generic patchset as an alternative.

Thoughts?

Thanks,

Nicolai

[1] https://lpc.events/event/17/contributions/1541/
[2] https://lore.kernel.org/r/20231110170428.6664-1-pmladek@suse.com

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)