Message ID | 156773434815.31441.12739136439382289412.stgit@devnote2 (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | x86: kprobes: Prohibit kprobes on Xen/KVM emulate prefixes | expand |
On Fri, Sep 06, 2019 at 10:45:48AM +0900, Masami Hiramatsu wrote: > diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h > index 62ca03ef5c65..fe33a9798708 100644 > --- a/arch/x86/include/asm/xen/interface.h > +++ b/arch/x86/include/asm/xen/interface.h > @@ -379,12 +379,15 @@ struct xen_pmu_arch { > * Prefix forces emulation of some non-trapping instructions. > * Currently only CPUID. > */ > +#include <asm/xen/prefix.h> > + > #ifdef __ASSEMBLY__ > -#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ; > +#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ; > #define XEN_CPUID XEN_EMULATE_PREFIX cpuid > #else > -#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; " > +#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; " > #define XEN_CPUID XEN_EMULATE_PREFIX "cpuid" > + > #endif Possibly you can do something like: #define XEN_EMULATE_PREFIX __ASM_FORM(.byte __XEN_EMULATE_PREFIX ;) #define XEN_CPUID XEN_EMULATE_PREFIX __ASM_FORM(cpuid) > #endif /* _ASM_X86_XEN_INTERFACE_H */ > diff --git a/arch/x86/include/asm/xen/prefix.h b/arch/x86/include/asm/xen/prefix.h > new file mode 100644 > index 000000000000..f901be0d7a95 > --- /dev/null > +++ b/arch/x86/include/asm/xen/prefix.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H > +#define _TOOLS_ASM_X86_XEN_PREFIX_H > + > +#include <linux/stringify.h> > + > +#define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e > +#define __XEN_EMULATE_PREFIX_STR __stringify(__XEN_EMULATE_PREFIX) > + > +#endif How about we make this asm/virt_prefix.h or something and include: /* * Virt escape sequences to trigger instruction emulation; * ideally these would decode to 'whole' instruction and not destroy * the instruction stream; sadly this is not true for the 'kvm' one :/ */ #define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e /* ud2 ; .ascii "xen" */ #define __KVM_EMULATE_PREFIX 0x0f,0x0b,0x6b,0x76,0x6d /* ud2 ; .ascii "kvm" */ > diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c > index 0b5862ba6a75..b7eb50187db9 100644 > --- a/arch/x86/lib/insn.c > +++ b/arch/x86/lib/insn.c > @@ -58,6 +61,37 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64) > insn->addr_bytes = 4; > } > > +static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX }; > +/* See handle_ud()@arch/x86/kvm/x86.c */ > +static const insn_byte_t kvm_prefix[] = "\xf\xbkvm"; Then you can make this consistent; maybe even something like: static const insn_byte_t *virt_prefix[] = { { __XEN_EMULATE_PREFIX }, { __KVM_EMULATE_PREFIX }, { NULL }, }; And then change emulate_prefix_size to emulate_prefix_index ?
On Fri, 6 Sep 2019 09:34:36 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Sep 06, 2019 at 10:45:48AM +0900, Masami Hiramatsu wrote: > > > diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h > > index 62ca03ef5c65..fe33a9798708 100644 > > --- a/arch/x86/include/asm/xen/interface.h > > +++ b/arch/x86/include/asm/xen/interface.h > > @@ -379,12 +379,15 @@ struct xen_pmu_arch { > > * Prefix forces emulation of some non-trapping instructions. > > * Currently only CPUID. > > */ > > +#include <asm/xen/prefix.h> > > + > > #ifdef __ASSEMBLY__ > > -#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ; > > +#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ; > > #define XEN_CPUID XEN_EMULATE_PREFIX cpuid > > #else > > -#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; " > > +#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; " > > #define XEN_CPUID XEN_EMULATE_PREFIX "cpuid" > > + > > #endif > > Possibly you can do something like: > > #define XEN_EMULATE_PREFIX __ASM_FORM(.byte __XEN_EMULATE_PREFIX ;) > #define XEN_CPUID XEN_EMULATE_PREFIX __ASM_FORM(cpuid) Hmm, OK. But should I split this change from insn decoder change? > > > #endif /* _ASM_X86_XEN_INTERFACE_H */ > > diff --git a/arch/x86/include/asm/xen/prefix.h b/arch/x86/include/asm/xen/prefix.h > > new file mode 100644 > > index 000000000000..f901be0d7a95 > > --- /dev/null > > +++ b/arch/x86/include/asm/xen/prefix.h > > @@ -0,0 +1,10 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H > > +#define _TOOLS_ASM_X86_XEN_PREFIX_H > > + > > +#include <linux/stringify.h> > > + > > +#define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e > > +#define __XEN_EMULATE_PREFIX_STR __stringify(__XEN_EMULATE_PREFIX) > > + > > +#endif > > How about we make this asm/virt_prefix.h or something and include: > > /* > * Virt escape sequences to trigger instruction emulation; > * ideally these would decode to 'whole' instruction and not destroy > * the instruction stream; sadly this is not true for the 'kvm' one :/ > */ > > #define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e /* ud2 ; .ascii "xen" */ > #define __KVM_EMULATE_PREFIX 0x0f,0x0b,0x6b,0x76,0x6d /* ud2 ; .ascii "kvm" */ OK, and in that case I think we should do this in separated patch from this change. > > > diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c > > index 0b5862ba6a75..b7eb50187db9 100644 > > --- a/arch/x86/lib/insn.c > > +++ b/arch/x86/lib/insn.c > > > @@ -58,6 +61,37 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64) > > insn->addr_bytes = 4; > > } > > > > +static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX }; > > +/* See handle_ud()@arch/x86/kvm/x86.c */ > > +static const insn_byte_t kvm_prefix[] = "\xf\xbkvm"; > > Then you can make this consistent; maybe even something like: > > static const insn_byte_t *virt_prefix[] = { > { __XEN_EMULATE_PREFIX }, > { __KVM_EMULATE_PREFIX }, > { NULL }, > }; > > And then change emulate_prefix_size to emulate_prefix_index ? Hmm, how we can get the length of those emulate prefixes? For struct insn, since the size information is important, other sub fields have "nbyte" field so that caller can find the actual bytes from original byte stream. Maybe we can have struct emulate_prefix { .nbyte, .type }; and define enum etc.. but for me, it seems a bit over engineering. (since no one use that feature) Thank you,
On Fri, 6 Sep 2019 17:45:19 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > How about we make this asm/virt_prefix.h or something and include: > > > > /* > > * Virt escape sequences to trigger instruction emulation; > > * ideally these would decode to 'whole' instruction and not destroy > > * the instruction stream; sadly this is not true for the 'kvm' one :/ > > */ > > > > #define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e /* ud2 ; .ascii "xen" */ > > #define __KVM_EMULATE_PREFIX 0x0f,0x0b,0x6b,0x76,0x6d /* ud2 ; .ascii "kvm" */ BTW, what should we call it, "emulate prefix" or "virt prefix"? "virt prefix" sounds too generic to me. So I rather like emulate_prefix.h. Thank you,
On Fri, Sep 06, 2019 at 05:51:43PM +0900, Masami Hiramatsu wrote: > On Fri, 6 Sep 2019 17:45:19 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > > > > How about we make this asm/virt_prefix.h or something and include: > > > > > > /* > > > * Virt escape sequences to trigger instruction emulation; > > > * ideally these would decode to 'whole' instruction and not destroy > > > * the instruction stream; sadly this is not true for the 'kvm' one :/ > > > */ > > > > > > #define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e /* ud2 ; .ascii "xen" */ > > > #define __KVM_EMULATE_PREFIX 0x0f,0x0b,0x6b,0x76,0x6d /* ud2 ; .ascii "kvm" */ > > BTW, what should we call it, "emulate prefix" or "virt prefix"? > "virt prefix" sounds too generic to me. So I rather like emulate_prefix.h. Works for me; and yeah, just see what is best for the other things. I only started down that road because the Xen and KVM 'prefixes' were initialized so inconsistently.
On Fri, 6 Sep 2019 09:34:36 +0200 Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Sep 06, 2019 at 10:45:48AM +0900, Masami Hiramatsu wrote: > > > diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h > > index 62ca03ef5c65..fe33a9798708 100644 > > --- a/arch/x86/include/asm/xen/interface.h > > +++ b/arch/x86/include/asm/xen/interface.h > > @@ -379,12 +379,15 @@ struct xen_pmu_arch { > > * Prefix forces emulation of some non-trapping instructions. > > * Currently only CPUID. > > */ > > +#include <asm/xen/prefix.h> > > + > > #ifdef __ASSEMBLY__ > > -#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ; > > +#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ; > > #define XEN_CPUID XEN_EMULATE_PREFIX cpuid > > #else > > -#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; " > > +#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; " > > #define XEN_CPUID XEN_EMULATE_PREFIX "cpuid" > > + > > #endif > > Possibly you can do something like: > > #define XEN_EMULATE_PREFIX __ASM_FORM(.byte __XEN_EMULATE_PREFIX ;) > #define XEN_CPUID XEN_EMULATE_PREFIX __ASM_FORM(cpuid) Oops, this doesn't work, since __ASM_FORM(x) uses #x directly # define __ASM_FORM(x) " " #x " " which doesn't expand "x" if x is a macro. We have to use __stringify like # include <linux/stringify.h> # define __ASM_FORM(x) " " __stringify(x) " " So this needs aonther patch in the series :) Thank you,
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h index 154f27be8bfc..5c1ae3eff9d4 100644 --- a/arch/x86/include/asm/insn.h +++ b/arch/x86/include/asm/insn.h @@ -45,6 +45,7 @@ struct insn { struct insn_field immediate2; /* for 64bit imm or seg16 */ }; + int emulate_prefix_size; insn_attr_t attr; unsigned char opnd_bytes; unsigned char addr_bytes; @@ -128,6 +129,11 @@ static inline int insn_is_evex(struct insn *insn) return (insn->vex_prefix.nbytes == 4); } +static inline int insn_has_emulate_prefix(struct insn *insn) +{ + return !!insn->emulate_prefix_size; +} + /* Ensure this instruction is decoded completely */ static inline int insn_complete(struct insn *insn) { diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h index 62ca03ef5c65..fe33a9798708 100644 --- a/arch/x86/include/asm/xen/interface.h +++ b/arch/x86/include/asm/xen/interface.h @@ -379,12 +379,15 @@ struct xen_pmu_arch { * Prefix forces emulation of some non-trapping instructions. * Currently only CPUID. */ +#include <asm/xen/prefix.h> + #ifdef __ASSEMBLY__ -#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ; +#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ; #define XEN_CPUID XEN_EMULATE_PREFIX cpuid #else -#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; " +#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; " #define XEN_CPUID XEN_EMULATE_PREFIX "cpuid" + #endif #endif /* _ASM_X86_XEN_INTERFACE_H */ diff --git a/arch/x86/include/asm/xen/prefix.h b/arch/x86/include/asm/xen/prefix.h new file mode 100644 index 000000000000..f901be0d7a95 --- /dev/null +++ b/arch/x86/include/asm/xen/prefix.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H +#define _TOOLS_ASM_X86_XEN_PREFIX_H + +#include <linux/stringify.h> + +#define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e +#define __XEN_EMULATE_PREFIX_STR __stringify(__XEN_EMULATE_PREFIX) + +#endif diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c index 0b5862ba6a75..b7eb50187db9 100644 --- a/arch/x86/lib/insn.c +++ b/arch/x86/lib/insn.c @@ -13,6 +13,9 @@ #include <asm/inat.h> #include <asm/insn.h> +/* For special Xen prefix */ +#include <asm/xen/prefix.h> + /* Verify next sizeof(t) bytes can be on the same instruction */ #define validate_next(t, insn, n) \ ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr) @@ -58,6 +61,37 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64) insn->addr_bytes = 4; } +static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX }; +/* See handle_ud()@arch/x86/kvm/x86.c */ +static const insn_byte_t kvm_prefix[] = "\xf\xbkvm"; + +static int __insn_get_emulate_prefix(struct insn *insn, + const insn_byte_t *prefix, size_t len) +{ + size_t i; + + for (i = 0; i < len; i++) { + if (peek_nbyte_next(insn_byte_t, insn, i) != prefix[i]) + goto err_out; + } + + insn->emulate_prefix_size = len; + insn->next_byte += len; + + return 1; + +err_out: + return 0; +} + +static void insn_get_emulate_prefix(struct insn *insn) +{ + if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix))) + return; + + __insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix)); +} + /** * insn_get_prefixes - scan x86 instruction prefix bytes * @insn: &struct insn containing instruction @@ -76,6 +110,8 @@ void insn_get_prefixes(struct insn *insn) if (prefixes->got) return; + insn_get_emulate_prefix(insn); + nb = 0; lb = 0; b = peek_next(insn_byte_t, insn); diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h index 37a4c390750b..568854b14d0a 100644 --- a/tools/arch/x86/include/asm/insn.h +++ b/tools/arch/x86/include/asm/insn.h @@ -45,6 +45,7 @@ struct insn { struct insn_field immediate2; /* for 64bit imm or seg16 */ }; + int emulate_prefix_size; insn_attr_t attr; unsigned char opnd_bytes; unsigned char addr_bytes; @@ -128,6 +129,11 @@ static inline int insn_is_evex(struct insn *insn) return (insn->vex_prefix.nbytes == 4); } +static inline int insn_has_emulate_prefix(struct insn *insn) +{ + return !!insn->emulate_prefix_size; +} + /* Ensure this instruction is decoded completely */ static inline int insn_complete(struct insn *insn) { diff --git a/tools/arch/x86/include/asm/xen/prefix.h b/tools/arch/x86/include/asm/xen/prefix.h new file mode 100644 index 000000000000..f901be0d7a95 --- /dev/null +++ b/tools/arch/x86/include/asm/xen/prefix.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H +#define _TOOLS_ASM_X86_XEN_PREFIX_H + +#include <linux/stringify.h> + +#define __XEN_EMULATE_PREFIX 0x0f,0x0b,0x78,0x65,0x6e +#define __XEN_EMULATE_PREFIX_STR __stringify(__XEN_EMULATE_PREFIX) + +#endif diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c index 79e048f1d902..ce04e43e0749 100644 --- a/tools/arch/x86/lib/insn.c +++ b/tools/arch/x86/lib/insn.c @@ -13,6 +13,9 @@ #include "../include/asm/inat.h" #include "../include/asm/insn.h" +/* For special Xen prefix */ +#include "../include/asm/xen/prefix.h" + /* Verify next sizeof(t) bytes can be on the same instruction */ #define validate_next(t, insn, n) \ ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr) @@ -58,6 +61,37 @@ void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64) insn->addr_bytes = 4; } +static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX }; +/* See handle_ud()@arch/x86/kvm/x86.c */ +static const insn_byte_t kvm_prefix[] = "\xf\xbkvm"; + +static int __insn_get_emulate_prefix(struct insn *insn, + const insn_byte_t *prefix, size_t len) +{ + size_t i; + + for (i = 0; i < len; i++) { + if (peek_nbyte_next(insn_byte_t, insn, i) != prefix[i]) + goto err_out; + } + + insn->emulate_prefix_size = len; + insn->next_byte += len; + + return 1; + +err_out: + return 0; +} + +static void insn_get_emulate_prefix(struct insn *insn) +{ + if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix))) + return; + + __insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix)); +} + /** * insn_get_prefixes - scan x86 instruction prefix bytes * @insn: &struct insn containing instruction @@ -76,6 +110,8 @@ void insn_get_prefixes(struct insn *insn) if (prefixes->got) return; + insn_get_emulate_prefix(insn); + nb = 0; lb = 0; b = peek_next(insn_byte_t, insn); diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh index 0a832e265a50..34143ea3d477 100755 --- a/tools/objtool/sync-check.sh +++ b/tools/objtool/sync-check.sh @@ -4,6 +4,7 @@ FILES=' arch/x86/include/asm/inat_types.h arch/x86/include/asm/orc_types.h +arch/x86/include/asm/xen/prefix.h arch/x86/lib/x86-opcode-map.txt arch/x86/tools/gen-insn-attr-x86.awk ' @@ -46,6 +47,6 @@ done check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"' check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"' check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"' -check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]"' +check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/xen/prefix.h[\">]"' cd - diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh index e2e0f06c97d0..edcffa55a826 100755 --- a/tools/perf/check-headers.sh +++ b/tools/perf/check-headers.sh @@ -115,7 +115,7 @@ check lib/ctype.c '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B check arch/x86/include/asm/inat.h '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"' check arch/x86/include/asm/insn.h '-I "^#include [\"<]\(asm/\)*inat.h[\">]"' check arch/x86/lib/inat.c '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"' -check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]"' +check arch/x86/lib/insn.c '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/xen/prefix.h[\">]"' # diff non-symmetric files check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl