On Wed, Mar 6, 2019 at 2:13 PM Peter Zijlstra <pet...@infradead.org> wrote:
>
> On Fri, Mar 01, 2019 at 04:23:05PM +0100, Peter Zijlstra wrote:
>
> > But yes, I'll try some annotation, see what that looks like.
>
> OK; that took a lot of time.. and a number of objtool bugs fixed but I
> think I have something that I don't hate -- although it is not as solid
> as I'd like it to be.
>
>
> unmodified:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000 150: 48 b8 ff ff ff ff ff movabs $0xffff7fffffffffff,%rax
> 0007 157: 7f ff ff
> 000a 15a: 48 8b 0c 24 mov (%rsp),%rcx
> 000e 15e: 48 39 c7 cmp %rax,%rdi
> 0011 161: 76 23 jbe 186 <__asan_load1+0x36>
> 0013 163: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
> 001a 16a: fc ff df
> 001d 16d: 48 89 fa mov %rdi,%rdx
> 0020 170: 48 c1 ea 03 shr $0x3,%rdx
> 0024 174: 0f b6 04 02 movzbl (%rdx,%rax,1),%eax
> 0028 178: 84 c0 test %al,%al
> 002a 17a: 75 01 jne 17d <__asan_load1+0x2d>
> 002c 17c: c3 retq
> 002d 17d: 89 fa mov %edi,%edx
> 002f 17f: 83 e2 07 and $0x7,%edx
> 0032 182: 38 d0 cmp %dl,%al
> 0034 184: 7f f6 jg 17c <__asan_load1+0x2c>
> 0036 186: 31 d2 xor %edx,%edx
> 0038 188: be 01 00 00 00 mov $0x1,%esi
> 003d 18d: e9 00 00 00 00 jmpq 192 <__asan_load1+0x42>
> 003e 18e: R_X86_64_PLT32 kasan_report-0x4
>
> exception:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000 150: 48 b8 ff ff ff ff ff movabs $0xffff7fffffffffff,%rax
> 0007 157: 7f ff ff
> 000a 15a: 48 8b 0c 24 mov (%rsp),%rcx
> 000e 15e: 48 39 c7 cmp %rax,%rdi
> 0011 161: 76 23 jbe 186 <__asan_load1+0x36>
> 0013 163: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
> 001a 16a: fc ff df
> 001d 16d: 48 89 fa mov %rdi,%rdx
> 0020 170: 48 c1 ea 03 shr $0x3,%rdx
> 0024 174: 0f b6 04 02 movzbl (%rdx,%rax,1),%eax
> 0028 178: 84 c0 test %al,%al
> 002a 17a: 75 01 jne 17d <__asan_load1+0x2d>
> 002c 17c: c3 retq
> 002d 17d: 89 fa mov %edi,%edx
> 002f 17f: 83 e2 07 and $0x7,%edx
> 0032 182: 38 d0 cmp %dl,%al
> 0034 184: 7f f6 jg 17c <__asan_load1+0x2c>
> 0036 186: 65 48 8b 04 25 00 00 mov %gs:0x0,%rax
> 003d 18d: 00 00
> 003b 18b: R_X86_64_32S current_task
> 003f 18f: 8b 80 68 08 00 00 mov 0x868(%rax),%eax
> 0045 195: 85 c0 test %eax,%eax
> 0047 197: 75 e3 jne 17c <__asan_load1+0x2c>
> 0049 199: be 01 00 00 00 mov $0x1,%esi
> 004e 19e: 31 d2 xor %edx,%edx
> 0050 1a0: 0f 0b ud2
> 0052 1a2: c3 retq
>
> annotated:
>
> 0000 0000000000000150 <__asan_load1>:
> 0000 150: 48 b8 ff ff ff ff ff movabs $0xffff7fffffffffff,%rax
> 0007 157: 7f ff ff
> 000a 15a: 53 push %rbx
/\/\/\/\/\/\
This push is unpleasant on hot fast path. I think we need to move
whole report cold path into a separate noinline function as it is now,
and that function will do the magic with smap. Then this won't prevent
tail calling and won't affect fast-path codegen.
> 000b 15b: 48 8b 4c 24 08 mov 0x8(%rsp),%rcx
> 0010 160: 48 39 c7 cmp %rax,%rdi
> 0013 163: 76 24 jbe 189 <__asan_load1+0x39>
> 0015 165: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
> 001c 16c: fc ff df
> 001f 16f: 48 89 fa mov %rdi,%rdx
> 0022 172: 48 c1 ea 03 shr $0x3,%rdx
> 0026 176: 0f b6 04 02 movzbl (%rdx,%rax,1),%eax
> 002a 17a: 84 c0 test %al,%al
> 002c 17c: 75 02 jne 180 <__asan_load1+0x30>
> 002e 17e: 5b pop %rbx
> 002f 17f: c3 retq
> 0030 180: 89 fa mov %edi,%edx
> 0032 182: 83 e2 07 and $0x7,%edx
> 0035 185: 38 d0 cmp %dl,%al
> 0037 187: 7f f5 jg 17e <__asan_load1+0x2e>
> 0039 189: 9c pushfq
> 003a 18a: 5b pop %rbx
> 003b 18b: 90 nop
> 003c 18c: 90 nop
> 003d 18d: 90 nop
> 003e 18e: 31 d2 xor %edx,%edx
> 0040 190: be 01 00 00 00 mov $0x1,%esi
> 0045 195: e8 00 00 00 00 callq 19a <__asan_load1+0x4a>
> 0046 196: R_X86_64_PLT32 __kasan_report-0x4
> 004a 19a: 53 push %rbx
> 004b 19b: 9d popfq
> 004c 19c: 5b pop %rbx
> 004d 19d: c3 retq
>
>
> ---
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -28,6 +28,23 @@
> #ifdef CONFIG_KASAN
> void __init kasan_early_init(void);
> void __init kasan_init(void);
> +
> +#include <asm/smap.h>
> +
> +extern void __kasan_report(unsigned long addr, size_t size, bool is_write,
> unsigned long ip);
> +
> +static __always_inline
> +void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned
> long ip)
> +{
> + unsigned long flags;
> +
> + flags = smap_save();
Previously you said that messing with smap here causes boot errors.
Shouldn't we do smap_save iff kasan_report_enabled? Otherwise we just
bail out, so no need to enable/disable smap.
> + __kasan_report(addr, size, is_write, ip);
> + smap_restore(flags);
> +
> +}
> +#define kasan_report kasan_report
> +
> #else
> static inline void kasan_early_init(void) { }
> static inline void kasan_init(void) { }
> --- a/arch/x86/include/asm/smap.h
> +++ b/arch/x86/include/asm/smap.h
> @@ -46,6 +46,8 @@
>
> #ifdef CONFIG_X86_SMAP
>
> +#include <asm/irqflags.h>
> +
> static __always_inline void clac(void)
> {
> /* Note: a barrier is implicit in alternative() */
> @@ -58,6 +60,18 @@ static __always_inline void stac(void)
> alternative("", __stringify(__ASM_STAC), X86_FEATURE_SMAP);
> }
>
> +static __always_inline unsigned long smap_save(void)
> +{
> + unsigned long flags = arch_local_save_flags();
> + clac();
> + return flags;
> +}
> +
> +static __always_inline void smap_restore(unsigned long flags)
> +{
> + arch_local_irq_restore(flags);
> +}
> +
> /* These macros can be used in asm() statements */
> #define ASM_CLAC \
> ALTERNATIVE("", __stringify(__ASM_CLAC), X86_FEATURE_SMAP)
> @@ -69,6 +83,9 @@ static __always_inline void stac(void)
> static inline void clac(void) { }
> static inline void stac(void) { }
>
> +static inline unsigned long smap_save(void) { return 0; }
> +static inline void smap_restore(unsigned long flags) { }
> +
> #define ASM_CLAC
> #define ASM_STAC
>
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -83,6 +83,8 @@ size_t kasan_metadata_size(struct kmem_c
> bool kasan_save_enable_multi_shot(void);
> void kasan_restore_multi_shot(bool enabled);
>
> +void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned
> long ip);
> +
> #else /* CONFIG_KASAN */
>
> static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -153,8 +155,14 @@ static inline void kasan_remove_zero_sha
> static inline void kasan_unpoison_slab(const void *ptr) { }
> static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return
> 0; }
>
> +static inline void __kasan_report(unsigned long addr, size_t size, bool
> is_write, unsigned long ip) { }
> +
> #endif /* CONFIG_KASAN */
>
> +#ifndef kasan_report
> +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size,
> is_write, ip)
> +#endif
> +
> #ifdef CONFIG_KASAN_GENERIC
>
> #define KASAN_SHADOW_INIT 0
> @@ -177,9 +185,6 @@ void kasan_init_tags(void);
>
> void *kasan_reset_tag(const void *addr);
>
> -void kasan_report(unsigned long addr, size_t size,
> - bool is_write, unsigned long ip);
> -
> #else /* CONFIG_KASAN_SW_TAGS */
>
> static inline void kasan_init_tags(void) { }
> --- a/mm/kasan/generic_report.c
> +++ b/mm/kasan/generic_report.c
> @@ -118,14 +118,14 @@ const char *get_bug_type(struct kasan_ac
> #define DEFINE_ASAN_REPORT_LOAD(size) \
> void __asan_report_load##size##_noabort(unsigned long addr) \
> { \
> - kasan_report(addr, size, false, _RET_IP_); \
> + __kasan_report(addr, size, false, _RET_IP_); \
Unless I am missing something, this seems to make this patch no-op. We
fixed kasan_report for smap, but here we now use __kasan_report which
is not fixed. So this won't work with smap again?..
> } \
> EXPORT_SYMBOL(__asan_report_load##size##_noabort)
>
> #define DEFINE_ASAN_REPORT_STORE(size) \
> void __asan_report_store##size##_noabort(unsigned long addr) \
> { \
> - kasan_report(addr, size, true, _RET_IP_); \
> + __kasan_report(addr, size, true, _RET_IP_); \
> } \
> EXPORT_SYMBOL(__asan_report_store##size##_noabort)
>
> @@ -142,12 +142,12 @@ DEFINE_ASAN_REPORT_STORE(16);
>
> void __asan_report_load_n_noabort(unsigned long addr, size_t size)
> {
> - kasan_report(addr, size, false, _RET_IP_);
> + __kasan_report(addr, size, false, _RET_IP_);
> }
> EXPORT_SYMBOL(__asan_report_load_n_noabort);
>
> void __asan_report_store_n_noabort(unsigned long addr, size_t size)
> {
> - kasan_report(addr, size, true, _RET_IP_);
> + __kasan_report(addr, size, true, _RET_IP_);
> }
> EXPORT_SYMBOL(__asan_report_store_n_noabort);
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -130,8 +130,6 @@ void check_memory_region(unsigned long a
> void *find_first_bad_addr(void *addr, size_t size);
> const char *get_bug_type(struct kasan_access_info *info);
>
> -void kasan_report(unsigned long addr, size_t size,
> - bool is_write, unsigned long ip);
> void kasan_report_invalid_free(void *object, unsigned long ip);
>
> #if defined(CONFIG_KASAN_GENERIC) && \
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
> end_report(&flags);
> }
>
> -void kasan_report(unsigned long addr, size_t size,
> +void __kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip)
> {
> struct kasan_access_info info;
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -43,6 +43,7 @@ enum op_dest_type {
> OP_DEST_REG_INDIRECT,
> OP_DEST_MEM,
> OP_DEST_PUSH,
> + OP_DEST_PUSHF,
> OP_DEST_LEAVE,
> };
>
> @@ -57,6 +58,7 @@ enum op_src_type {
> OP_SRC_REG_INDIRECT,
> OP_SRC_CONST,
> OP_SRC_POP,
> + OP_SRC_POPF,
> OP_SRC_ADD,
> OP_SRC_AND,
> };
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -357,13 +357,13 @@ int arch_decode_instruction(struct elf *
> /* pushf */
> *type = INSN_STACK;
> op->src.type = OP_SRC_CONST;
> - op->dest.type = OP_DEST_PUSH;
> + op->dest.type = OP_DEST_PUSHF;
> break;
>
> case 0x9d:
> /* popf */
> *type = INSN_STACK;
> - op->src.type = OP_SRC_POP;
> + op->src.type = OP_SRC_POPF;
> op->dest.type = OP_DEST_MEM;
> break;
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1359,11 +1359,11 @@ static int update_insn_state_regs(struct
> return 0;
>
> /* push */
> - if (op->dest.type == OP_DEST_PUSH)
> + if (op->dest.type == OP_DEST_PUSH || op->dest.type == OP_DEST_PUSHF)
> cfa->offset += 8;
>
> /* pop */
> - if (op->src.type == OP_SRC_POP)
> + if (op->src.type == OP_SRC_POP || op->src.type == OP_SRC_POPF)
> cfa->offset -= 8;
>
> /* add immediate to sp */
> @@ -1620,6 +1620,7 @@ static int update_insn_state(struct inst
> break;
>
> case OP_SRC_POP:
> + case OP_SRC_POPF:
> if (!state->drap && op->dest.type == OP_DEST_REG &&
> op->dest.reg == cfa->base) {
>
> @@ -1684,6 +1685,7 @@ static int update_insn_state(struct inst
> break;
>
> case OP_DEST_PUSH:
> + case OP_DEST_PUSHF:
> state->stack_size += 8;
> if (cfa->base == CFI_SP)
> cfa->offset += 8;
> @@ -1774,7 +1776,7 @@ static int update_insn_state(struct inst
> break;
>
> case OP_DEST_MEM:
> - if (op->src.type != OP_SRC_POP) {
> + if (op->src.type != OP_SRC_POP && op->src.type !=
> OP_SRC_POPF) {
> WARN_FUNC("unknown stack-related memory operation",
> insn->sec, insn->offset);
> return -1;
> @@ -2071,6 +2073,16 @@ static int validate_branch(struct objtoo
> if (update_insn_state(insn, &state))
> return 1;
>
> + if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
> + if (state.uaccess)
> + state.uaccess_stack++;
> + }
> +
> + if (insn->stack_op.src.type == OP_SRC_POPF) {
> + if (state.uaccess_stack &&
> !--state.uaccess_stack)
> + state.uaccess =
> func_uaccess_safe(func);
> + }
> +
> break;
>
> case INSN_STAC:
> @@ -2088,7 +2100,7 @@ static int validate_branch(struct objtoo
> return 1;
> }
>
> - if (func_uaccess_safe(func)) {
> + if (func_uaccess_safe(func) && !state.uaccess_stack) {
> WARN_FUNC("UACCESS-safe disables UACCESS",
> sec, insn->offset);
> return 1;
> }
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -32,6 +32,7 @@ struct insn_state {
> unsigned char type;
> bool bp_scratch;
> bool drap, end, uaccess;
> + int uaccess_stack;
> int drap_reg, drap_offset;
> struct cfi_reg vals[CFI_NUM_REGS];
> };
