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

[Kees Cook]

On Wed, Jul 19, 2017 at 12:37 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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