refcount: Report UAF for refcount_sub_and_test(0) when counter==0

[PATCH] refcount: Report UAF for refcount_sub_and_test(0) when counter==0

Posted by Petr Pavlu 1 year, 5 months ago

When a reference counter is at zero and refcount_sub_and_test() is invoked
to subtract zero, the function accepts this request without any warning and
returns true. This behavior does not seem ideal because the counter being
already at zero indicates a use-after-free. Furthermore, returning true by
refcount_sub_and_test() in this case potentially results in a double-free
done by its caller.

Modify the underlying function __refcount_sub_and_test() to warn about this
case as a use-after-free and have it return false to avoid the potential
double-free.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---

Motivation for this patch is an earlier kretprobe problem described at:
https://lore.kernel.org/linux-trace-kernel/92cff289-facb-4e42-b761-6fd2515d6018@suse.com/

 drivers/misc/lkdtm/refcount.c | 16 ++++++++++++++++
 include/linux/refcount.h      |  4 ++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/lkdtm/refcount.c b/drivers/misc/lkdtm/refcount.c
index 5cd488f54cfa..8f744bee6fbd 100644
--- a/drivers/misc/lkdtm/refcount.c
+++ b/drivers/misc/lkdtm/refcount.c
@@ -182,6 +182,21 @@ static void lkdtm_REFCOUNT_SUB_AND_TEST_NEGATIVE(void)
 	check_negative(&neg, 3);
 }
 
+/*
+ * A refcount_sub_and_test() by zero when the counter is at zero should act like
+ * refcount_sub_and_test() above when going negative.
+ */
+static void lkdtm_REFCOUNT_SUB_AND_TEST_ZERO(void)
+{
+	refcount_t neg = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount_sub_and_test() at zero\n");
+	if (refcount_sub_and_test(0, &neg))
+		pr_warn("Weird: refcount_sub_and_test() reported zero\n");
+
+	check_negative(&neg, 0);
+}
+
 static void check_from_zero(refcount_t *ref)
 {
 	switch (refcount_read(ref)) {
@@ -400,6 +415,7 @@ static struct crashtype crashtypes[] = {
 	CRASHTYPE(REFCOUNT_DEC_NEGATIVE),
 	CRASHTYPE(REFCOUNT_DEC_AND_TEST_NEGATIVE),
 	CRASHTYPE(REFCOUNT_SUB_AND_TEST_NEGATIVE),
+	CRASHTYPE(REFCOUNT_SUB_AND_TEST_ZERO),
 	CRASHTYPE(REFCOUNT_INC_ZERO),
 	CRASHTYPE(REFCOUNT_ADD_ZERO),
 	CRASHTYPE(REFCOUNT_INC_SATURATED),
diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index 59b3b752394d..35f039ecb272 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -266,12 +266,12 @@ bool __refcount_sub_and_test(int i, refcount_t *r, int *oldp)
 	if (oldp)
 		*oldp = old;
 
-	if (old == i) {
+	if (old > 0 && old == i) {
 		smp_acquire__after_ctrl_dep();
 		return true;
 	}
 
-	if (unlikely(old < 0 || old - i < 0))
+	if (unlikely(old <= 0 || old - i < 0))
 		refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
 
 	return false;

base-commit: 2df0193e62cf887f373995fb8a91068562784adc
-- 
2.35.3

Re: [PATCH] refcount: Report UAF for refcount_sub_and_test(0) when counter==0

Posted by Kees Cook 1 year, 5 months ago

On Wed, 17 Jul 2024 15:00:23 +0200, Petr Pavlu wrote:
> When a reference counter is at zero and refcount_sub_and_test() is invoked
> to subtract zero, the function accepts this request without any warning and
> returns true. This behavior does not seem ideal because the counter being
> already at zero indicates a use-after-free. Furthermore, returning true by
> refcount_sub_and_test() in this case potentially results in a double-free
> done by its caller.
> 
> [...]

(Peter, let me know if you'd rather this went via a different tree...)

Applied to for-linus/hardening, thanks!

[1/1] refcount: Report UAF for refcount_sub_and_test(0) when counter==0
      https://git.kernel.org/kees/c/cee1fb1bb8bd

Take care,

-- 
Kees Cook

Re: [PATCH] refcount: Report UAF for refcount_sub_and_test(0) when counter==0

Posted by Peter Zijlstra 1 year, 5 months ago

On Wed, Jul 17, 2024 at 03:00:23PM +0200, Petr Pavlu wrote:
> When a reference counter is at zero and refcount_sub_and_test() is invoked
> to subtract zero, the function accepts this request without any warning and
> returns true. This behavior does not seem ideal because the counter being
> already at zero indicates a use-after-free. Furthermore, returning true by
> refcount_sub_and_test() in this case potentially results in a double-free
> done by its caller.
> 
> Modify the underlying function __refcount_sub_and_test() to warn about this
> case as a use-after-free and have it return false to avoid the potential
> double-free.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
> 
> Motivation for this patch is an earlier kretprobe problem described at:
> https://lore.kernel.org/linux-trace-kernel/92cff289-facb-4e42-b761-6fd2515d6018@suse.com/

Well that's good fun.... :/

The patch seems sane enough to me, I don't think add() has a similar
issue, adding 0 is still daft, but as is I don't think it will actually
misbehave. Notably add_not_zero(0) will fail when 0 and succeed (with
no effect) when !0.

Still, adding or subtracting 0 is pretty stupid, so perhaps we should
more explicitly warn on that?

Anyway, for this patch:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

>  drivers/misc/lkdtm/refcount.c | 16 ++++++++++++++++
>  include/linux/refcount.h      |  4 ++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/refcount.c b/drivers/misc/lkdtm/refcount.c
> index 5cd488f54cfa..8f744bee6fbd 100644
> --- a/drivers/misc/lkdtm/refcount.c
> +++ b/drivers/misc/lkdtm/refcount.c
> @@ -182,6 +182,21 @@ static void lkdtm_REFCOUNT_SUB_AND_TEST_NEGATIVE(void)
>  	check_negative(&neg, 3);
>  }
>  
> +/*
> + * A refcount_sub_and_test() by zero when the counter is at zero should act like
> + * refcount_sub_and_test() above when going negative.
> + */
> +static void lkdtm_REFCOUNT_SUB_AND_TEST_ZERO(void)
> +{
> +	refcount_t neg = REFCOUNT_INIT(0);
> +
> +	pr_info("attempting bad refcount_sub_and_test() at zero\n");
> +	if (refcount_sub_and_test(0, &neg))
> +		pr_warn("Weird: refcount_sub_and_test() reported zero\n");
> +
> +	check_negative(&neg, 0);
> +}
> +
>  static void check_from_zero(refcount_t *ref)
>  {
>  	switch (refcount_read(ref)) {
> @@ -400,6 +415,7 @@ static struct crashtype crashtypes[] = {
>  	CRASHTYPE(REFCOUNT_DEC_NEGATIVE),
>  	CRASHTYPE(REFCOUNT_DEC_AND_TEST_NEGATIVE),
>  	CRASHTYPE(REFCOUNT_SUB_AND_TEST_NEGATIVE),
> +	CRASHTYPE(REFCOUNT_SUB_AND_TEST_ZERO),
>  	CRASHTYPE(REFCOUNT_INC_ZERO),
>  	CRASHTYPE(REFCOUNT_ADD_ZERO),
>  	CRASHTYPE(REFCOUNT_INC_SATURATED),
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index 59b3b752394d..35f039ecb272 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -266,12 +266,12 @@ bool __refcount_sub_and_test(int i, refcount_t *r, int *oldp)
>  	if (oldp)
>  		*oldp = old;
>  
> -	if (old == i) {
> +	if (old > 0 && old == i) {
>  		smp_acquire__after_ctrl_dep();
>  		return true;
>  	}
>  
> -	if (unlikely(old < 0 || old - i < 0))
> +	if (unlikely(old <= 0 || old - i < 0))
>  		refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
>  
>  	return false;
> 
> base-commit: 2df0193e62cf887f373995fb8a91068562784adc
> -- 
> 2.35.3
>