(no subject)

[raschupkin.ri]

[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);

[PATCH 1/2] [PATCH] livepatch: support of modifying refcount_t without underflow after unpatch.

[raschupkin.ri]

From: Roman Rashchupkin <raschupkin.ri@gmail.com>

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.

Signed-off-by: Roman Rashchupkin <raschupkin.ri@gmail.com>
---
 include/linux/livepatch_refcount.h | 19 +++++++
 kernel/livepatch/Makefile          |  2 +-
 kernel/livepatch/kprefcount.c      | 89 ++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/livepatch_refcount.h
 create mode 100644 kernel/livepatch/kprefcount.c

diff --git a/include/linux/livepatch_refcount.h b/include/linux/livepatch_refcount.h
new file mode 100644
index 000000000000..02f9e7eeadb2
--- /dev/null
+++ b/include/linux/livepatch_refcount.h
@@ -0,0 +1,19 @@
+#ifndef __KP_REFCOUNT_T__
+#define __KP_REFCOUNT_T__
+
+#include <linux/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);
+
+#endif
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index cf03d4bdfc66..8ff0926372c2 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
-livepatch-objs := core.o patch.o shadow.o state.o transition.o
+livepatch-objs := core.o patch.o shadow.o state.o transition.o kprefcount.o
diff --git a/kernel/livepatch/kprefcount.c b/kernel/livepatch/kprefcount.c
new file mode 100644
index 000000000000..6878033c5ddc
--- /dev/null
+++ b/kernel/livepatch/kprefcount.c
@@ -0,0 +1,89 @@
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/version.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/refcount.h>
+#include <linux/livepatch_refcount.h>
+
+MODULE_LICENSE("GPL");
+
+kprefcount_t *kprefcount_alloc(refcount_t *refcount, gfp_t flags)
+{
+	kprefcount_t *kp_ref = kmalloc(sizeof(kprefcount_t), flags);
+	if (!kp_ref)
+		return 0;
+	kp_ref->refcount = refcount;
+	refcount_set(&kp_ref->kprefcount, 1);
+	spin_lock_init(&kp_ref->lock);
+	return kp_ref;
+}
+EXPORT_SYMBOL(kprefcount_alloc);
+
+void kprefcount_free(kprefcount_t *kp_ref)
+{
+	kfree(kp_ref);
+}
+EXPORT_SYMBOL(kprefcount_free);
+
+static bool kprefcount_check_owner(unsigned char *ref_holder, int kprefholder_flag)
+{
+	if (!ref_holder)
+		return true;
+	return (*ref_holder) & kprefholder_flag;
+}
+
+int kprefcount_read(kprefcount_t *kp_ref)
+{
+	return refcount_read(kp_ref->refcount) + refcount_read(&kp_ref->kprefcount) - 1;
+}
+EXPORT_SYMBOL(kprefcount_read);
+
+void kprefcount_inc(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag)
+{
+	spin_lock(&kp_ref->lock);
+	BUG_ON(ref_holder && kprefcount_check_owner(ref_holder, kprefholder_flag));
+	if (ref_holder)
+		*ref_holder |= kprefholder_flag;
+	refcount_inc(&kp_ref->kprefcount);
+	spin_unlock(&kp_ref->lock);
+}
+EXPORT_SYMBOL(kprefcount_inc);
+
+static int kprefcount_dec_locked(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag)
+{
+	if (!kprefcount_check_owner(ref_holder, kprefholder_flag))
+		return -1;
+	if (ref_holder) {
+		*ref_holder &= !kprefholder_flag;
+		refcount_dec(&kp_ref->kprefcount);
+	} else
+		refcount_dec(kp_ref->refcount);
+	return 0;
+}
+
+void kprefcount_dec(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag)
+{
+	spin_lock(&kp_ref->lock);
+	kprefcount_dec_locked(kp_ref, ref_holder, kprefholder_flag);
+	spin_unlock(&kp_ref->lock);
+
+}
+EXPORT_SYMBOL(kprefcount_dec);
+
+bool kprefcount_dec_and_test(kprefcount_t *kp_ref, unsigned char *ref_holder, int kprefholder_flag)
+{
+	spin_lock(&kp_ref->lock);
+	if (kprefcount_dec_locked(kp_ref, ref_holder, kprefholder_flag)) {
+		spin_unlock(&kp_ref->lock);
+		return false;
+	}
+	if (kprefcount_read(kp_ref) == 0) {
+		spin_unlock(&kp_ref->lock);
+		return true;
+	}
+	spin_unlock(&kp_ref->lock);
+	return false;
+}
+EXPORT_SYMBOL(kprefcount_dec_and_test);
-- 
2.43.0

[PATCH 2/2] selftests/livepatch: Add tests for kprefcount_t support

[raschupkin.ri]

From: Roman Rashchupkin <raschupkin.ri@gmail.com>

Signed-off-by: Roman Rashchupkin <raschupkin.ri@gmail.com>
---
 tools/testing/selftests/livepatch/Makefile    |   3 +-
 .../selftests/livepatch/test-kprefcount.sh    |  16 +++
 .../selftests/livepatch/test_modules/Makefile |   4 +-
 .../test_modules/test_klp_kprefcount.c        | 120 ++++++++++++++++++
 .../test_modules/test_klp_refcount.c          |  65 ++++++++++
 5 files changed, 206 insertions(+), 2 deletions(-)
 create mode 100755 tools/testing/selftests/livepatch/test-kprefcount.sh
 create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_kprefcount.c
 create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_refcount.c

diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
index 35418a4790be..48926ebc77f2 100644
--- a/tools/testing/selftests/livepatch/Makefile
+++ b/tools/testing/selftests/livepatch/Makefile
@@ -10,7 +10,8 @@ TEST_PROGS := \
 	test-state.sh \
 	test-ftrace.sh \
 	test-sysfs.sh \
-	test-syscall.sh
+	test-syscall.sh \
+	test-kprefcount.sh
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/livepatch/test-kprefcount.sh b/tools/testing/selftests/livepatch/test-kprefcount.sh
new file mode 100755
index 000000000000..8ea6c18f59dd
--- /dev/null
+++ b/tools/testing/selftests/livepatch/test-kprefcount.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+insmod test_modules/test_klp_refcount.ko
+insmod test_modules/test_klp_kprefcount.ko
+livepatch_enabled=/sys/kernel/livepatch/test_klp_kprefcount/enabled
+while [ ! -e $livepatch_enabled -o $(cat $livepatch_enabled) -eq 0 ]; do
+	sleep 0.01;
+done
+echo 0 > $livepatch_enabled
+while [ $(cat $livepatch_enabled) -eq 1 ]; do
+	sleep 0.01;
+done
+while [ -e $livepatch_enabled ]; do
+	sleep 0.01;
+done
+rmmod test_klp_kprefcount
+rmmod test_klp_refcount
diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile
index e6e638c4bcba..c26797372e0d 100644
--- a/tools/testing/selftests/livepatch/test_modules/Makefile
+++ b/tools/testing/selftests/livepatch/test_modules/Makefile
@@ -11,7 +11,9 @@ obj-m += test_klp_atomic_replace.o \
 	test_klp_state2.o \
 	test_klp_state3.o \
 	test_klp_shadow_vars.o \
-	test_klp_syscall.o
+	test_klp_syscall.o \
+	test_klp_refcount.o \
+	test_klp_kprefcount.o
 
 # Ensure that KDIR exists, otherwise skip the compilation
 modules:
diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_kprefcount.c b/tools/testing/selftests/livepatch/test_modules/test_klp_kprefcount.c
new file mode 100644
index 000000000000..063f286ebcec
--- /dev/null
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_kprefcount.c
@@ -0,0 +1,120 @@
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/livepatch.h>
+#include <linux/livepatch_refcount.h>
+
+extern refcount_t test_refcount;
+extern int TEST_LIVEPATCH_KPREFCOUNT;
+
+#define ITER 100
+#define NREF 10
+#define KP_NREF 10
+static struct ref_holder {
+	unsigned char v;
+} kp_ref_holders[KP_NREF] = { 0 };
+kprefcount_t *kp_test_ref = 0;
+
+static int livepatch_refcount_test_iter(void)
+{
+	int k, i;
+	for (k=0; k<ITER; k++) {
+		for (i=0; i<KP_NREF; i++)
+			refcount_inc(&test_refcount);
+		for (i=0; i<KP_NREF; i++)
+			if (kprefcount_dec_and_test(kp_test_ref, 0, 1)) {
+				pr_alert("livepatch refcount underflow\n");
+				return -1;
+			}
+	}
+	TEST_LIVEPATCH_KPREFCOUNT = 0;
+	return 0;
+}
+
+struct delayed_work kp_work_refcount;
+static void kp_test_refcount(struct work_struct *work)
+{
+	int i, k;
+	for (k=0; k<ITER; k++) {
+		for (i=0; i<KP_NREF; i++)
+			kprefcount_dec(kp_test_ref, &kp_ref_holders[i].v, 1);
+		// Intentional refcounter underflow for additional testing
+		for (i=0; i<KP_NREF-1; i++)
+			kprefcount_inc(kp_test_ref, &kp_ref_holders[i].v, 1);
+	}
+}
+
+static void kp_post_patch_callback(struct klp_object *klp_obj)
+{
+	schedule_delayed_work(&kp_work_refcount, 0);
+}
+
+static void kp_pre_unpatch_callback(struct klp_object *klp_obj)
+{
+	cancel_delayed_work_sync(&kp_work_refcount);
+}
+
+static struct klp_func funcs[] = {
+	{
+		.old_name = "refcount_test_iter",
+		.new_func = livepatch_refcount_test_iter,
+	}, { }
+};
+
+static struct klp_object objs[] = {
+	{
+		.name = "test_klp_refcount",
+		.funcs = funcs,
+		.callbacks = {
+			.post_patch = kp_post_patch_callback,
+			.pre_unpatch = kp_pre_unpatch_callback,
+		},
+	}, { }
+};
+
+static struct klp_patch patch = {
+	.mod = THIS_MODULE,
+	.objs = objs,
+};
+
+struct delayed_work work_refcount;
+
+static void do_test_refcount(struct work_struct *work)
+{
+	int i;
+	for (i=0; i<NREF; i++) {
+		if (refcount_read(&test_refcount) <= 1)
+			pr_info("LIVEPATCH refcount test done.\n");
+			return;
+		refcount_dec(&test_refcount);
+		if (refcount_read(&test_refcount) < 0)
+			pr_alert("post-livepatch refcount underflow\n");
+	}
+	for (i=0; i<NREF; i++)
+		refcount_inc(&test_refcount);
+}
+
+static int refcount_test_init(void)
+{
+	int ret;
+	kp_test_ref = kprefcount_alloc(&test_refcount, GFP_KERNEL);
+	if (!kp_test_ref) {
+		pr_alert("kprefcount_livepatch: memory allocation_failed");
+		return -1;
+	}
+	ret = klp_enable_patch(&patch);
+	INIT_DELAYED_WORK(&kp_work_refcount, kp_test_refcount);
+	return 0;
+}
+
+static void refcount_test_exit(void)
+{
+}
+
+module_init(refcount_test_init);
+module_exit(refcount_test_exit);
+MODULE_INFO(livepatch, "Y");
+MODULE_AUTHOR("Roman Rashchupkin <raschupkin.ri@gmail.com>");
+MODULE_DESCRIPTION("Livepatch test: kprefcount");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_refcount.c b/tools/testing/selftests/livepatch/test_modules/test_klp_refcount.c
new file mode 100644
index 000000000000..bd9c57e63476
--- /dev/null
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_refcount.c
@@ -0,0 +1,65 @@
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/livepatch.h>
+#include <linux/livepatch_refcount.h>
+
+#define ITER 100
+#define NREF 10
+int TEST_LIVEPATCH_KPREFCOUNT = 1;
+EXPORT_SYMBOL(TEST_LIVEPATCH_KPREFCOUNT);
+refcount_t test_refcount = REFCOUNT_INIT(1);
+EXPORT_SYMBOL(test_refcount);
+
+int refcount_test_iter(void)
+{
+	int i;
+	for (i=0; i<NREF; i++)
+		refcount_inc(&test_refcount);
+	for (i=0; i<NREF; i++)
+		if (refcount_dec_and_test(&test_refcount))
+			return -1;
+	return 0;
+}
+
+int refcount_test(void)
+{
+	int i;
+	for (i=0; i<ITER; i++) {
+		if (refcount_test_iter())
+			return -1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(refcount_test);
+
+struct delayed_work kp_test_work;
+void start_refcount_test(struct work_struct *work)
+{
+	if (!TEST_LIVEPATCH_KPREFCOUNT)
+		return;
+	if (refcount_test()) {
+		pr_alert(KERN_ERR "refcount_test: error.\n");
+		return;
+	}
+	schedule_delayed_work(&kp_test_work, msecs_to_jiffies(50));
+}
+
+static int refcount_test_init(void)
+{
+	INIT_DELAYED_WORK(&kp_test_work, start_refcount_test);
+	schedule_delayed_work(&kp_test_work, msecs_to_jiffies(50));
+	return 0;
+}
+
+static void refcount_test_exit(void)
+{
+	cancel_delayed_work_sync(&kp_test_work);
+}
+
+module_init(refcount_test_init);
+module_exit(refcount_test_exit);
+MODULE_AUTHOR("Roman Rashchupkin <raschupkin.ri@gmail.com>");
+MODULE_DESCRIPTION("Livepatch test: refcount");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 1/2] [PATCH] livepatch: support of modifying refcount_t without underflow after unpatch.

[Jeff Johnson]

On 7/14/24 12:59, raschupkin.ri@gmail.com wrote:
> From: Roman Rashchupkin <raschupkin.ri@gmail.com>
> 
> 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.
> 
> Signed-off-by: Roman Rashchupkin <raschupkin.ri@gmail.com>
> ---
>   include/linux/livepatch_refcount.h | 19 +++++++
>   kernel/livepatch/Makefile          |  2 +-
>   kernel/livepatch/kprefcount.c      | 89 ++++++++++++++++++++++++++++++
>   3 files changed, 109 insertions(+), 1 deletion(-)
>   create mode 100644 include/linux/livepatch_refcount.h
>   create mode 100644 kernel/livepatch/kprefcount.c
> 
> diff --git a/include/linux/livepatch_refcount.h b/include/linux/livepatch_refcount.h
> new file mode 100644
> index 000000000000..02f9e7eeadb2
> --- /dev/null
> +++ b/include/linux/livepatch_refcount.h
> @@ -0,0 +1,19 @@
> +#ifndef __KP_REFCOUNT_T__
> +#define __KP_REFCOUNT_T__
> +
> +#include <linux/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);
> +
> +#endif
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
> index cf03d4bdfc66..8ff0926372c2 100644
> --- a/kernel/livepatch/Makefile
> +++ b/kernel/livepatch/Makefile
> @@ -1,4 +1,4 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   obj-$(CONFIG_LIVEPATCH) += livepatch.o
>   
> -livepatch-objs := core.o patch.o shadow.o state.o transition.o
> +livepatch-objs := core.o patch.o shadow.o state.o transition.o kprefcount.o
> diff --git a/kernel/livepatch/kprefcount.c b/kernel/livepatch/kprefcount.c
> new file mode 100644
> index 000000000000..6878033c5ddc
> --- /dev/null
> +++ b/kernel/livepatch/kprefcount.c
> @@ -0,0 +1,89 @@
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/version.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/refcount.h>
> +#include <linux/livepatch_refcount.h>
> +
> +MODULE_LICENSE("GPL");
> +

Note that "make W=1" will generate a warning if a module doesn't have a 
MODULE_DESCRIPTION().

I've been fixing the existing warnings tree-wide and am hoping to 
prevent new instances from appearing.

One way I've been doing this is by searching lore for patches which add 
a MODULE_LICENSE() but which do not add a MODULE_DESCRIPTION() since 
that is a common sign of a missing description.

But in this specific case it seems the MODULE_LICENSE() may be the issue 
since I don't see how kprefcount.c could ever be built as a module. So 
in this specific case it seems as if the MODULE_LICENSE() should be removed.

Note that none of the other kernel/livepatch/*.c files have a 
MODULE_LICENSE(), and CONFIG_LIVEPATCH is a bool and hence does not 
support being built as a module.

/jeff

Re:

[Joe Lawrence]

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

Re:

[Roman Rashchupkin]

Hello.
About upstream commits creating live-patching for which this API would 
facilitate,
I could reference several CVEs:
- CVE-2023-5633
     "drm/vmwgfx: Keep a gem reference to user bos in surfaces"
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=91398b413d03660fd5828f7b4abc64e884b98069

  drm_gem_object_get(&vbo->tbo.base);/drm_gem_object_put(&tmp_buf->tbo.base);

- CVE-2023-6932
     "ipv4: igmp: fix refcnt uaf issue when receiving igmp query packet"
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e2b706c69190

     refcount_inc_not_zero(&im->refcnt)/ip_ma_put(im);

- CVE-2022-20566
     "Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put"
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d0be8347c623e0ac4202a1d4e0373882821f56b0

     kref_get_unless_zero(&c->kref)/l2cap_chan_put(chan)

Only in all of these 3 cases I can remember now, refcount_t is mostly 
used inside wrapper-functions, and from the top of my head now I don't 
remember CVEs that plainly add refcount_inc()/dec().
In case the proposed patch is merged, maybe CVE-2023-5633 would be 
suited best for documentation, or source git could be searched for 
better example.

Two types of problems that you classify, are exactly what I'm attempting 
to solve for added refcount_inc/dec() in the code that is added by 
live-patch. Let's continue with your numbering (1) and (2) for 
simplicity of discussion.

Concerning problem (1), shadow variables are certainly could be used 
instead of my refholder bit in reference-holder structures. That's only 
my preference for simplicity that live-patches code is so often lacking, 
to use one bit in existing structures instead of hash-table based shadow 
variables. But certainly shadow-variables are also a good approach, and 
could be used instead of mine (unsigned char *ref_holder, int 
kprefholder_flag) in the kprefcount_t API.

About problem (2), iterating through all shadow-variable/refholder 
instances would also work, but it is just unnecessary processing during 
unpatch.
In my approach the second kprefcount variable with lifetime of 
live-patch being applied is used, it provides correct refcounting during 
live-patch, but the main idea is that this variable can be just safely 
removed at the unpatch.
The only complication could be values of refholder bits, that must be 
reset at live-patch apply, or probably it is more simple to implement at 
the unpatch, as all kprefcount_t structs are allocated by patch-code.
---
Roman Rashchupkin

On 7/15/24 22:20, Joe Lawrence wrote:
> 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
>

Re:

[Nicolai Stange]

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)

Re:

[Roman Rashchupkin]

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

 > Same.

I just don't have enough experience using live-patch shadow-variables, 
so I agree that probably that's a better general solution for problem 
(1) of refcount underflow, than mine refholder flags.

 > 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].

About (2) of incorrect refcount_t value left after unpatch, it seems as 
a bit different and more special problem with counters, compared to the 
support of live-patch states, which can be solved for refcount_t in a 
simpler way.

IMHO adding temporary kprefcount_t variable for the time of live-patch 
being applied, modifying only this kprefcount_t variable by all added in 
livepatch inc()/dec(), provides correct refcounting during live-patch is 
applied. Then at the unpatch this temporaray variable can just safely be 
discarded. This way the only additional code to live-patch would be 
functions with original refcount_dec_and_test() calls.

---

Roman Rashchupkin

On 7/16/24 11:28, Nicolai Stange wrote:
> 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
>

Re:

[Song Liu]

On Mon, Jul 15, 2024 at 4:00 AM <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);

IIUC, kprefcount alone is not enough to solve the two issues. We still
need some mechanism to manage the "ref_holder". Shadow variable
is probably the best option here.

The primary idea here is to enhance the refcount with a map. This
may be too expensive in term memory consumption in some use
cases.

Overall, I don't think this change adds much more value on top of
shadow variable.

Thanks,
Song

Re:

[Joe Lawrence]

On 7/16/24 05:53, Roman Rashchupkin wrote:
>>> The first thing that comes to mind is that this might be solved using
>>> the existing shadow variable API.
> 
>> Same.
> 
> I just don't have enough experience using live-patch shadow-variables,
> so I agree that probably that's a better general solution for problem
> (1) of refcount underflow, than mine refholder flags.
> 

Yes, a general solution could cover the same problem but for different
datatypes, including locks, mutex, etc.

>> 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].

Thanks for the link -- I thought of that grand-unified
shadow/callback/states patch but couldn't find the latest version.  (I
see that Miroslav has just resurrected it with a fresh review, too.)

>> 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?
>>

I definitely think the states API enhancement could be used to handle
the cases here via shadow variables.

In the meantime, are you using the kprefcount_t API currently via a
livepatch support module?  i.e. we don't need this in the kernel asap to
solve these problems, right?

-- 
Joe