Re: [PATCH v3 22/22] x86/int3: Ensure that poke_int3_handler() is not sanitized

[Dmitry Vyukov <dvyukov@google.com>]

On Wed, Feb 19, 2020 at 4:14 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> In order to ensure poke_int3_handler() is completely self contained --
> we call this while we're modifying other text, imagine the fun of
> hitting another INT3 -- ensure that everything is without sanitize
> crud.

+kasan-dev

Hi Peter,

How do we hit another INT3 here? Does the code do
out-of-bounds/use-after-free writes?
Debugging later silent memory corruption may be no less fun :)

Not sanitizing bsearch entirely is a bit unfortunate. We won't find
any bugs in it when called from other sites too.
It may deserve a comment at least. Tomorrow I may want to remove
__no_sanitize, just because sanitizing more is better, and no int3
test will fail to stop me from doing that...

It's quite fragile. Tomorrow poke_int3_handler handler calls more of
fewer functions, and both ways it's not detected by anything. And if
we ignore all by one function, it is still not helpful, right?
Depending on failure cause/mode, using kasan_disable/enable_current
may be a better option.


> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/alternative.c       |    4 ++--
>  arch/x86/kernel/traps.c             |    2 +-
>  include/linux/compiler-clang.h      |    7 +++++++
>  include/linux/compiler-gcc.h        |    6 ++++++
>  include/linux/compiler.h            |    5 +++++
>  include/linux/compiler_attributes.h |    1 +
>  lib/bsearch.c                       |    2 +-
>  7 files changed, 23 insertions(+), 4 deletions(-)
>
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -979,7 +979,7 @@ static __always_inline void *text_poke_a
>         return _stext + tp->rel_addr;
>  }
>
> -static int notrace patch_cmp(const void *key, const void *elt)
> +static int notrace __no_sanitize patch_cmp(const void *key, const void *elt)
>  {
>         struct text_poke_loc *tp = (struct text_poke_loc *) elt;
>
> @@ -991,7 +991,7 @@ static int notrace patch_cmp(const void
>  }
>  NOKPROBE_SYMBOL(patch_cmp);
>
> -int notrace poke_int3_handler(struct pt_regs *regs)
> +int notrace __no_sanitize poke_int3_handler(struct pt_regs *regs)
>  {
>         struct bp_patching_desc *desc;
>         struct text_poke_loc *tp;
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -496,7 +496,7 @@ dotraplinkage void do_general_protection
>  }
>  NOKPROBE_SYMBOL(do_general_protection);
>
> -dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
> +dotraplinkage void notrace __no_sanitize do_int3(struct pt_regs *regs, long error_code)
>  {
>         if (poke_int3_handler(regs))
>                 return;
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -24,6 +24,13 @@
>  #define __no_sanitize_address
>  #endif
>
> +#if __has_feature(undefined_sanitizer)
> +#define __no_sanitize_undefined \
> +               __atribute__((no_sanitize("undefined")))
> +#else
> +#define __no_sanitize_undefined
> +#endif
> +
>  /*
>   * Not all versions of clang implement the the type-generic versions
>   * of the builtin overflow checkers. Fortunately, clang implements
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -145,6 +145,12 @@
>  #define __no_sanitize_address
>  #endif
>
> +#if __has_attribute(__no_sanitize_undefined__)
> +#define __no_sanitize_undefined __attribute__((no_sanitize_undefined))
> +#else
> +#define __no_sanitize_undefined
> +#endif
> +
>  #if GCC_VERSION >= 50100
>  #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
>  #endif
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -199,6 +199,7 @@ void __read_once_size(const volatile voi
>         __READ_ONCE_SIZE;
>  }
>
> +#define __no_kasan __no_sanitize_address
>  #ifdef CONFIG_KASAN
>  /*
>   * We can't declare function 'inline' because __no_sanitize_address confilcts
> @@ -274,6 +275,10 @@ static __always_inline void __write_once
>   */
>  #define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
>
> +#define __no_ubsan __no_sanitize_undefined
> +
> +#define __no_sanitize __no_kasan __no_ubsan
> +
>  static __no_kasan_or_inline
>  unsigned long read_word_at_a_time(const void *addr)
>  {
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -41,6 +41,7 @@
>  # define __GCC4_has_attribute___nonstring__           0
>  # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
>  # define __GCC4_has_attribute___fallthrough__         0
> +# define __GCC4_has_attribute___no_sanitize_undefined__ (__GNUC_MINOR__ >= 9)
>  #endif
>
>  /*
> --- a/lib/bsearch.c
> +++ b/lib/bsearch.c
> @@ -28,7 +28,7 @@
>   * the key and elements in the array are of the same type, you can use
>   * the same comparison function for both sort() and bsearch().
>   */
> -void *bsearch(const void *key, const void *base, size_t num, size_t size,
> +void __no_sanitize *bsearch(const void *key, const void *base, size_t num, size_t size,
>               cmp_func_t cmp)
>  {
>         const char *pivot;
>
>