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

[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

(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

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