| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 10 Oct 2019 13:48:20 -0700
From: Kees Cook <keescook@...omium.org>
To: Will Deacon <will@...nel.org>
Cc: linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
Elena Reshetova <elena.reshetova@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Hanjun Guo <guohanjun@...wei.com>,
Jan Glauber <jglauber@...vell.com>
Subject: Re: [PATCH v3 06/10] lib/refcount: Move saturation warnings out of
line
On Mon, Oct 07, 2019 at 04:46:59PM +0100, Will Deacon wrote:
> Having the refcount saturation and warnings inline bloats the text,
> despite the fact that these paths should never be executed in normal
> operation.
>
> Move the refcount saturation and warnings out of line to reduce the
> image size when refcount_t checking is enabled. Relative to an x86_64
> defconfig, the sizes reported by bloat-o-meter are:
>
> # defconfig+REFCOUNT_FULL, inline saturation (i.e. before this patch)
> Total: Before=14762076, After=14915442, chg +1.04%
>
> # defconfig+REFCOUNT_FULL, out-of-line saturation (i.e. after this patch)
> Total: Before=14762076, After=14835497, chg +0.50%
The downside of this change is that this means we get one warning per
refcount_saturation_type, where as before it was once per refcount
usage. I think, ultimately, this is okay, but it is a behavioral change
that might be worth pointing out.
>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Elena Reshetova <elena.reshetova@...el.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Will Deacon <will@...nel.org>
> ---
> include/linux/refcount.h | 39 ++++++++++++++++++++-------------------
> lib/refcount.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index 7f9aa6511142..40ddff74a96c 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -23,6 +23,16 @@ typedef struct refcount_struct {
>
> #define REFCOUNT_INIT(n) { .refs = ATOMIC_INIT(n), }
>
> +enum refcount_saturation_type {
> + REFCOUNT_ADD_NOT_ZERO_OVF,
> + REFCOUNT_ADD_OVF,
> + REFCOUNT_ADD_UAF,
> + REFCOUNT_SUB_UAF,
> + REFCOUNT_DEC_LEAK,
> +};
> +
> +void refcount_warn_saturate(refcount_t *r, enum refcount_saturation_type t);
> +
> /**
> * refcount_set - set a refcount's value
> * @r: the refcount
> @@ -116,10 +126,8 @@ static inline __must_check bool refcount_add_not_zero(int i, refcount_t *r)
> break;
> } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i));
>
> - if (unlikely(old < 0 || old + i < 0)) {
> - refcount_set(r, REFCOUNT_SATURATED);
> - WARN_ONCE(1, "refcount_t: saturated; leaking memory.\n");
> - }
> + if (unlikely(old < 0 || old + i < 0))
> + refcount_warn_saturate(r, REFCOUNT_ADD_NOT_ZERO_OVF);
>
> return old;
> }
> @@ -144,11 +152,10 @@ static inline void refcount_add(int i, refcount_t *r)
> {
> int old = atomic_fetch_add_relaxed(i, &r->refs);
>
> - WARN_ONCE(!old, "refcount_t: addition on 0; use-after-free.\n");
> - if (unlikely(old <= 0 || old + i <= 0)) {
> - refcount_set(r, REFCOUNT_SATURATED);
> - WARN_ONCE(old, "refcount_t: saturated; leaking memory.\n");
> - }
> + if (unlikely(!old))
> + refcount_warn_saturate(r, REFCOUNT_ADD_UAF);
> + else if (unlikely(old < 0 || old + i < 0))
> + refcount_warn_saturate(r, REFCOUNT_ADD_OVF);
> }
>
> /**
> @@ -215,10 +222,8 @@ static inline __must_check bool refcount_sub_and_test(int i, refcount_t *r)
> return true;
> }
>
> - if (unlikely(old - i < 0)) {
> - refcount_set(r, REFCOUNT_SATURATED);
> - WARN_ONCE(1, "refcount_t: underflow; use-after-free.\n");
> - }
> + if (unlikely(old - i < 0))
> + refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
>
> return false;
> }
> @@ -253,12 +258,8 @@ static inline __must_check bool refcount_dec_and_test(refcount_t *r)
> */
> static inline void refcount_dec(refcount_t *r)
> {
> - int old = atomic_fetch_sub_release(1, &r->refs);
> -
> - if (unlikely(old <= 1)) {
> - refcount_set(r, REFCOUNT_SATURATED);
> - WARN_ONCE(1, "refcount_t: decrement hit 0; leaking memory.\n");
> - }
> + if (unlikely(atomic_fetch_sub_release(1, &r->refs) <= 1))
> + refcount_warn_saturate(r, REFCOUNT_DEC_LEAK);
> }
> #else /* CONFIG_REFCOUNT_FULL */
>
> diff --git a/lib/refcount.c b/lib/refcount.c
> index 3a534fbebdcc..6a61d39f9390 100644
> --- a/lib/refcount.c
> +++ b/lib/refcount.c
> @@ -8,6 +8,34 @@
> #include <linux/spinlock.h>
> #include <linux/bug.h>
>
> +#define REFCOUNT_WARN(str) WARN_ONCE(1, "refcount_t: " str ".\n")
This define seems like overkill for just adding a prefix to 5 uses...
> +
> +void refcount_warn_saturate(refcount_t *r, enum refcount_saturation_type t)
> +{
> + refcount_set(r, REFCOUNT_SATURATED);
> +
> + switch (t) {
> + case REFCOUNT_ADD_NOT_ZERO_OVF:
> + REFCOUNT_WARN("saturated; leaking memory");
> + break;
> + case REFCOUNT_ADD_OVF:
> + REFCOUNT_WARN("saturated; leaking memory");
> + break;
> + case REFCOUNT_ADD_UAF:
> + REFCOUNT_WARN("addition on 0; use-after-free");
> + break;
> + case REFCOUNT_SUB_UAF:
> + REFCOUNT_WARN("underflow; use-after-free");
> + break;
> + case REFCOUNT_DEC_LEAK:
> + REFCOUNT_WARN("decrement hit 0; leaking memory");
Even the longest doesn't line wrap:
WARN_ONCE(1, "refcount_t: decrement hit 0; leaking memory\n");
> + break;
> + default:
> + WARN_ON(1);
This deserves a string too, yes?
WARN_ONCE(1, "refcount_t: unknown saturation event!?\n");
> + }
> +}
> +EXPORT_SYMBOL(refcount_warn_saturate);
> +
> /**
> * refcount_dec_if_one - decrement a refcount if it is 1
> * @r: the refcount
> --
> 2.23.0.581.g78d2f28ef7-goog
>
Otherwise, okay, I grudgingly accept the loss of warnings when running
the lkdtm tests in order to gain back some text size... :)
--
Kees Cook
Powered by blists - more mailing lists