From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751595AbdHaT1c (ORCPT ); Thu, 31 Aug 2017 15:27:32 -0400 Received: from mail-pg0-f49.google.com ([74.125.83.49]:38430 "EHLO mail-pg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750952AbdHaT1b (ORCPT ); Thu, 31 Aug 2017 15:27:31 -0400 X-Google-Smtp-Source: ADKCNb4rc62YicTdQzl0feEwgsZws6e2EDcPzhoJQZtYAPmHDDgS0ez2CtKhgytZtc/IM8zb9h51oA== Date: Thu, 31 Aug 2017 12:27:28 -0700 From: Kees Cook To: Mike Galbraith Cc: Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org Subject: [PATCH][DEBUG] x86/refcount: split up refcount saturation handling Message-ID: <20170831192728.GA135568@beast> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org In support of debugging the problems Mike Galbraith has seen with x86-refcount vs gcc vs network refcounts... This minimizes the differences between unchecked-refcount and x86-refcount by changing the refcount_dec() failure case to not saturate. The reporting of negative values is reduced to pr_warn from WARN to avoid spamming dmesg (which may impact race conditions). Ratelimiting is disabled just to be sure no reports are being dropped. Signed-off-by: Kees Cook --- arch/x86/mm/extable.c | 51 ++++++++++++++++++++++++++++++++++----------------- kernel/panic.c | 2 +- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index c076f710de4c..dcb498668370 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -43,19 +44,7 @@ EXPORT_SYMBOL_GPL(ex_handler_fault); bool ex_handler_refcount(const struct exception_table_entry *fixup, struct pt_regs *regs, int trapnr) { - /* First unconditionally saturate the refcount. */ - *(int *)regs->cx = INT_MIN / 2; - - /* - * Strictly speaking, this reports the fixup destination, not - * the fault location, and not the actually overflowing - * instruction, which is the instruction before the "js", but - * since that instruction could be a variety of lengths, just - * report the location after the overflow, which should be close - * enough for finding the overflow, as it's at least back in - * the function, having returned from .text.unlikely. - */ - regs->ip = ex_fixup_addr(fixup); + const char *msg; /* * This function has been called because either a negative refcount @@ -68,12 +57,40 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, * these cases we want a report, since it's a boundary condition. * */ - if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_ZF)) { - bool zero = regs->flags & X86_EFLAGS_ZF; - - refcount_error_report(regs, zero ? "hit zero" : "overflow"); + if (regs->flags & X86_EFLAGS_OF) { + /* Always saturate on overflow detection. */ + *(int *)regs->cx = INT_MIN / 2; + msg = "saturated due to overflow"; + } else if (regs->flags & X86_EFLAGS_SF) { + /* Do not generate traceback on re-saturation. */ + *(int *)regs->cx = INT_MIN / 2; + regs->ip = ex_fixup_addr(fixup); + pr_warn("refcount_t saturated due to negative result at %pB in %s[%d]\n", + (void *)instruction_pointer(regs), + current->comm, task_pid_nr(current)); + return true; + } else if (regs->flags & X86_EFLAGS_ZF) { + /* An unchecked dec-to-zero happened. WARN only. */ + msg = "hit zero without test"; + } else { + /* Impossible state. */ + *(int *)regs->cx = INT_MIN / 2; + msg = "saturated due to unknown state"; } + /* + * Strictly speaking, this reports the fixup destination, not + * the fault location, and not the actually overflowing + * instruction, which is the instruction before the "js", but + * since that instruction could be a variety of lengths, just + * report the location after the overflow, which should be close + * enough for finding the overflow, as it's at least back in + * the function, having returned from .text.unlikely. + */ + regs->ip = ex_fixup_addr(fixup); + + refcount_error_report(regs, msg); + return true; } EXPORT_SYMBOL_GPL(ex_handler_refcount); diff --git a/kernel/panic.c b/kernel/panic.c index bdd18afa19a4..966ade491543 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -605,7 +605,7 @@ EXPORT_SYMBOL(__stack_chk_fail); #ifdef CONFIG_ARCH_HAS_REFCOUNT void refcount_error_report(struct pt_regs *regs, const char *err) { - WARN_RATELIMIT(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n", + WARN(1, "refcount_t %s at %pB in %s[%d], uid/euid: %u/%u\n", err, (void *)instruction_pointer(regs), current->comm, task_pid_nr(current), from_kuid_munged(&init_user_ns, current_uid()), -- 2.7.4 -- Kees Cook Pixel Security