Re:

[Joe Lawrence <joe.lawrence@redhat.com>]

On Sun, Jul 14, 2024 at 09:59:32PM +0200, raschupkin.ri@gmail.com wrote:
> 
> [PATCH] livepatch: support of modifying refcount_t without underflow after unpatch
> 
> CVE fixes sometimes add refcount_inc/dec() pairs to the code with existing refcount_t.
> Two problems arise when applying live-patch in this case:
> 1) After refcount_t is being inc() during system is live-patched, after unpatch the counter value will not be valid, as corresponing dec() would never be called.
> 2) Underflows are possible in runtime in case dec() is called before corresponding inc() in the live-patched code.
> 
> Proposed kprefcount_t functions are using following approach to solve these two problems:
> 1) In addition to original refcount_t, temporary refcount_t is allocated, and after unpatch it is just removed. This way system is safe with correct refcounting while patch is applied, and no underflow would happend after unpatch.
> 2) For inc/dec() added by live-patch code, one bit in reference-holder structure is used (unsigned char *ref_holder, kprefholder_flag). In case dec() is called first, it is just ignored as ref_holder bit would still not be initialized.
> 
> 
> API is defined include/linux/livepatch_refcount.h:
> 
> typedef struct kprefcount_struct {
> 	refcount_t *refcount;
> 	refcount_t kprefcount;
> 	spinlock_t lock;
> } kprefcount_t;
> 
> kprefcount_t *kprefcount_alloc(refcount_t *refcount, gfp_t flags);
> void kprefcount_free(kprefcount_t *kp_ref);
> int kprefcount_read(kprefcount_t *kp_ref);
> void kprefcount_inc(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag);
> void kprefcount_dec(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag);
> bool kprefcount_dec_and_test(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag);
> 

Hi Roman,

Can you point to a specific upstream commit that this API facilitated a
livepatch conversion?  That would make a good addition to the
Documentation/livepatch/ side of a potential v2.

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


The first thing that comes to mind is that this might be solved using
the existing shadow variable API.  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 believe that would align the reference counts
with original kernel code expectations.

* note this approach probably requires atomic-replace livepatches, so
  only a single pre-unpatch hook is ever executed.


Also, the proposed patchset looks like it creates a parallel reference
counting structure... does this mean that the livepatch will need to
update *all* reference counting calls for the API to work (so points A,
B, B', and A' in my ascii-art above)?  This question loops back to my
first point about a real-world example that can be added to
Documentation/livepatch/, much like the ones found in the
shadow-vars.rst file.

Thanks,

--
Joe