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

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>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/r/20240717130023.5675-1-petr.pavlu@suse.com
Signed-off-by: Kees Cook <kees@kernel.org>

diff --git a/drivers/misc/lkdtm/refcount.c b/drivers/misc/lkdtm/refcount.c
index 5cd488f54cfa542ec3370030b0afcda2d75335c5..8f744bee6fbde83f196d71f0ce33447a0baeaeb5 100644 (file)
--- 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 59b3b752394d3cabd34e40f439eae513e4cdc2a5..35f039ecb2725618ca098e3515c6e19e2aece3ee 100644 (file)
--- 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;