kernel-hardening - Re: [PATCH v6 2/2] x86/refcount: Implement fast refcount overflow protection

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jKqqRJ3H0fwwc5W14U14p-QzTe1=UWJggA2yxLuj23ZTA@mail.gmail.com>
Date: Wed, 19 Jul 2017 12:45:19 -0700
From: Kees Cook <keescook@...omium.org>
To: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Ingo Molnar <mingo@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Christoph Hellwig <hch@...radead.org>, "Eric W. Biederman" <ebiederm@...ssion.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Jann Horn <jannh@...gle.com>, 
	Eric Biggers <ebiggers3@...il.com>, Elena Reshetova <elena.reshetova@...el.com>, 
	Hans Liljestrand <ishkamiel@...il.com>, Greg KH <gregkh@...uxfoundation.org>, 
	Alexey Dobriyan <adobriyan@...il.com>, "Serge E. Hallyn" <serge@...lyn.com>, arozansk@...hat.com, 
	Davidlohr Bueso <dave@...olabs.net>, Manfred Spraul <manfred@...orfullife.com>, 
	"axboe@...nel.dk" <axboe@...nel.dk>, James Bottomley <James.Bottomley@...senpartnership.com>, 
	"x86@...nel.org" <x86@...nel.org>, Arnd Bergmann <arnd@...db.de>, "David S. Miller" <davem@...emloft.net>, 
	Rik van Riel <riel@...hat.com>, LKML <linux-kernel@...r.kernel.org>, 
	linux-arch <linux-arch@...r.kernel.org>, 
	"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH v6 2/2] x86/refcount: Implement fast refcount overflow protection

On Wed, Jul 19, 2017 at 12:37 PM, Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> On Tue, Jul 18, 2017 at 05:03:34PM -0700, Kees Cook wrote:
>> +/*
>> + * Body of refcount error handling: in .text.unlikely, saved into CX the
>> + * address of the refcount that has entered a bad state, and trigger an
>> + * exception. Fixup address is back in regular execution flow in .text.
>> + */
>> +#define _REFCOUNT_EXCEPTION                          \
>> +     ".pushsection .text.unlikely\n"                 \
>> +     "111:\tlea %[counter], %%" _ASM_CX "\n"         \
>> +     "112:\t" ASM_UD0 "\n"                           \
>> +     ".popsection\n"                                 \
>> +     "113:\n"                                        \
>> +     _ASM_EXTABLE_REFCOUNT(112b, 113b)
>
> This confuses the freshly merged objtool 2.0, which is now too smart for
> its own good.  It's reporting some errors like:
>
>   >> kernel/sched/autogroup.o: warning: objtool: sched_autogroup_exit()+0x48: return with modified stack frame
>   >> kernel/sched/autogroup.o: warning: objtool: .text.unlikely+0x27: stack state mismatch: reg1[3]=-2-40 reg2[3]=-2-24
>   >> kernel/sched/autogroup.o: warning: objtool: sched_autogroup_exit()+0x14: stack state mismatch: reg1[3]=-2-40 reg2[3]=-2-24
>
> Because the UD instructions are used for both WARN and BUG, objtool
> doesn't know whether control flow continues past the instruction.  So in
> cases like this, it needs an "unreachable" annotation.
>
> Here's a patch to fix it, feel free to squash it into yours:

Thanks! I'll add it for v7.

> diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> index 13b91e850a02..e7587db3487c 100644
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -15,6 +15,7 @@
>         ".pushsection .text.unlikely\n"                 \
>         "111:\tlea %[counter], %%" _ASM_CX "\n"         \
>         "112:\t" ASM_UD0 "\n"                           \
> +       ASM_UNREACHABLE                                 \
>         ".popsection\n"                                 \
>         "113:\n"                                        \
>         _ASM_EXTABLE_REFCOUNT(112b, 113b)
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index cd4bbe8242bd..85e0b8f42ca0 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -202,15 +202,25 @@
>  #endif
>
>  #ifdef CONFIG_STACK_VALIDATION
> +
>  #define annotate_unreachable() ({                                      \
>         asm("%c0:\t\n"                                                  \
> -           ".pushsection .discard.unreachable\t\n"                     \
> -           ".long %c0b - .\t\n"                                        \
> -           ".popsection\t\n" : : "i" (__LINE__));                      \
> +           ".pushsection .discard.unreachable\n\t"                     \
> +           ".long %c0b - .\n\t"                                        \
> +           ".popsection\n\t" : : "i" (__LINE__));                      \

Is this just an indentation change?

>  })
> +
> +#define ASM_UNREACHABLE                                                        \
> +       "999: .pushsection .discard.unreachable\n\t"                    \
> +       ".long 999b - .\n\t"                                            \
> +       ".popsection\n\t"

Just so I understand, we'll get a single byte added for each exception
case, but it'll get discarded during final link?

> +
>  #else
> +
>  #define annotate_unreachable()
> -#endif
> +#define ASM_UNREACHABLE
> +
> +#endif /* CONFIG_STACK_VALIDATION */
>
>  /*
>   * Mark a position in code as unreachable.  This can be used to

Thanks!

-Kees

-- 
Kees Cook
Pixel Security
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.