[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jKoeB7tbxfcLC03tFnjYKYt+ZHtk_jruQJ887dPHJ-Gag@mail.gmail.com>
Date: Mon, 1 May 2017 10:36:59 -0700
From: Kees Cook <keescook@...omium.org>
To: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>,
PaX Team <pageexec@...email.hu>, Jann Horn <jannh@...gle.com>,
Eric Biggers <ebiggers3@...il.com>, Christoph Hellwig <hch@...radead.org>,
"axboe@...nel.dk" <axboe@...nel.dk>, James Bottomley <James.Bottomley@...senpartnership.com>,
Elena Reshetova <elena.reshetova@...el.com>, Hans Liljestrand <ishkamiel@...il.com>,
David Windsor <dwindsor@...il.com>, "x86@...nel.org" <x86@...nel.org>, Ingo Molnar <mingo@...nel.org>,
Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"David S. Miller" <davem@...emloft.net>, Rik van Riel <riel@...hat.com>,
linux-arch <linux-arch@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v2 2/2] x86, refcount: Implement fast refcount overflow protection
On Mon, May 1, 2017 at 9:30 AM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
>> +#define __REFCOUNT_EXCEPTION(size) \
>> + ".if "__stringify(size)" == 4\n\t" \
>> + ".pushsection .text.refcount_overflow\n" \
>> + ".elseif "__stringify(size)" == -4\n\t" \
>> + ".pushsection .text.refcount_underflow\n" \
>> + ".else\n" \
>> + ".error \"invalid size\"\n" \
>> + ".endif\n" \
>> + "111:\tlea %[counter],%%"_ASM_CX"\n\t" \
>> + "int $"__stringify(X86_REFCOUNT_VECTOR)"\n" \
>> + "222:\n\t" \
>> + ".popsection\n" \
>> + "333:\n" \
>> + _ASM_EXTABLE(222b, 333b)
>
> The 'size' argument doesn't seem to correspond to an actual size of
> anything. Its value '4' or '-4' only seems to indicate whether it's an
> overflow or an underflow.
This is to allow for expansion to refcount64_t if we ever move to it,
then we'll have 4 cases: 4, -4, 8, -8.
> Also there's some inconsistent use of "\n\t" on some lines, with "\n" on
> others.
It's not inconsistent, it's leaving directives at column 0, and
section and instructions at tab-stop 1.
>> +dotraplinkage void do_refcount_error(struct pt_regs *regs, long error_code)
>> +{
>> + const char *str = NULL;
>> +
>> + BUG_ON(!(regs->flags & X86_EFLAGS_SF));
>> +
>> +#define range_check(size, dir, type, value) \
>> + do { \
>> + if ((unsigned long)__##size##_##dir##_start <= regs->ip && \
>> + regs->ip < (unsigned long)__##size##_##dir##_end) { \
>> + *(type *)regs->cx = (value); \
>> + str = #size " " #dir; \
>> + } \
>> + } while (0)
>
> An interrupt was used, not a faulting exception, so regs->ip refers to
> the address *after* the 'int' instruction. So the beginning of the
> range should be exclusive, and the end of the range should be inclusive,
> like:
>
>> + if ((unsigned long)__##size##_##dir##_start < regs->ip && \
>> + regs->ip <= (unsigned long)__##size##_##dir##_end) { \
Ah, yes, good catch.
>> +
>> + /*
>> + * Reset to INT_MAX in both cases to attempt to let system
>> + * continue operating.
>> + */
>> + range_check(refcount, overflow, int, INT_MAX);
>> + range_check(refcount, underflow, int, INT_MAX);
>
> I think "range_check" doesn't adequately describe the macro. In
> addition to checking, it has a subtle side effect: it updates the
> counter value with INT_MAX.
>
> It's not clear why the 'size' argument has its name. Also, three of the
> arguments are always called with the same value. Anyway I suspect the
> code would be more readable if it were open coded without the macro.
Yeah, and I think I may drop the over/under distinction, since I think
I've convinced myself that we always need to reset to the same
position regardless of direction. This was originally for handling
generic atomic_t operations, not refcount_t... PeterZ may convince me
yet, but I'll send the next version without the over/under
distinction.
>> +#ifdef CONFIG_FAST_REFCOUNT
>> +static DEFINE_RATELIMIT_STATE(refcount_ratelimit, 15 * HZ, 3);
>> +
>> +void refcount_error_report(struct pt_regs *regs, const char *kind)
>> +{
>> + do_send_sig_info(SIGKILL, SEND_SIG_FORCED, current, true);
>> +
>> + if (!__ratelimit(&refcount_ratelimit))
>> + return;
>> +
>> + pr_emerg("%s detected in: %s:%d, uid/euid: %u/%u\n",
>> + kind ? kind : "refcount error",
>> + current->comm, task_pid_nr(current),
>> + from_kuid_munged(&init_user_ns, current_uid()),
>> + from_kuid_munged(&init_user_ns, current_euid()));
>> + print_symbol(KERN_EMERG "refcount error occurred at: %s\n",
>> + instruction_pointer(regs));
>> + preempt_disable();
>> + show_regs(regs);
>> + preempt_enable();
>> +}
>
> Why is preemption disabled before calling show_regs()?
I thought it was to avoid interleaving show_regs() output (I can't
think of a way regs would be externally modified).
>> +EXPORT_SYMBOL(refcount_error_report);
>
> Why is this exported? It looks like it's only called internally from
> traps.c.
Ah yes, good point. I'll drop this.
Thanks for the review!
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
