| Message ID | 156759754770.24473.11832897710080799131.stgit@devnote2 (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86: Prohibit kprobes on XEN_EMULATE_PREFIX | expand |
On 04/09/2019 12:45, Masami Hiramatsu wrote: > Hi, > > These patches allow x86 instruction decoder to decode > xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit > kprobes to probe on it. > > Josh reported that the objtool can not decode such special > prefixed instructions, and I found that we also have to > prohibit kprobes to probe on such instruction. > > This series can be applied on -tip master branch which > has merged Josh's objtool/perf sharing common x86 insn > decoder series. The paravirtualised xen-cpuid is were you'll see it most in a regular kernel, but be aware that it is also used for testing purposes in other circumstances, and there is an equivalent KVM prefix which is used for KVM testing. It might be better to generalise the decode support to "virtualisation escape prefix" or something slightly more generic. ~Andrew
On Wed, Sep 04, 2019 at 08:45:47PM +0900, Masami Hiramatsu wrote: > Hi, > > These patches allow x86 instruction decoder to decode > xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit > kprobes to probe on it. > > Josh reported that the objtool can not decode such special > prefixed instructions, and I found that we also have to > prohibit kprobes to probe on such instruction. > > This series can be applied on -tip master branch which > has merged Josh's objtool/perf sharing common x86 insn > decoder series. > > > Thank you, > > --- > > Masami Hiramatsu (2): > x86: xen: insn: Decode XEN_EMULATE_PREFIX correctly > x86: kprobes: Prohibit probing on instruction which has Xen prefix > > > arch/x86/include/asm/insn.h | 2 + > arch/x86/include/asm/xen/interface.h | 7 ++++- > arch/x86/include/asm/xen/prefix.h | 10 +++++++ > arch/x86/kernel/kprobes/core.c | 4 +++ > arch/x86/lib/insn.c | 43 +++++++++++++++++++++++++++++++ > tools/arch/x86/include/asm/insn.h | 2 + > tools/arch/x86/include/asm/xen/prefix.h | 10 +++++++ > tools/arch/x86/lib/insn.c | 43 +++++++++++++++++++++++++++++++ > tools/objtool/sync-check.sh | 3 +- > 9 files changed, 121 insertions(+), 3 deletions(-) > create mode 100644 arch/x86/include/asm/xen/prefix.h > create mode 100644 tools/arch/x86/include/asm/xen/prefix.h Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
On Wed, 4 Sep 2019 12:54:55 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 04/09/2019 12:45, Masami Hiramatsu wrote: > > Hi, > > > > These patches allow x86 instruction decoder to decode > > xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit > > kprobes to probe on it. > > > > Josh reported that the objtool can not decode such special > > prefixed instructions, and I found that we also have to > > prohibit kprobes to probe on such instruction. > > > > This series can be applied on -tip master branch which > > has merged Josh's objtool/perf sharing common x86 insn > > decoder series. > > The paravirtualised xen-cpuid is were you'll see it most in a regular > kernel, but be aware that it is also used for testing purposes in other > circumstances, and there is an equivalent KVM prefix which is used for > KVM testing. Good catch! I didn't notice that. Is that really same sequance or KVM uses another sequence of instructions for KVM prefix? > > It might be better to generalise the decode support to "virtualisation > escape prefix" or something slightly more generic. Agreed, it is easy to expand it, we can switch the prefix template. Could you tell me where I should look? I will add it. Thank you, > > ~Andrew
On 05/09/2019 02:49, Masami Hiramatsu wrote: > On Wed, 4 Sep 2019 12:54:55 +0100 > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 04/09/2019 12:45, Masami Hiramatsu wrote: >>> Hi, >>> >>> These patches allow x86 instruction decoder to decode >>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit >>> kprobes to probe on it. >>> >>> Josh reported that the objtool can not decode such special >>> prefixed instructions, and I found that we also have to >>> prohibit kprobes to probe on such instruction. >>> >>> This series can be applied on -tip master branch which >>> has merged Josh's objtool/perf sharing common x86 insn >>> decoder series. >> The paravirtualised xen-cpuid is were you'll see it most in a regular >> kernel, but be aware that it is also used for testing purposes in other >> circumstances, and there is an equivalent KVM prefix which is used for >> KVM testing. > Good catch! I didn't notice that. Is that really same sequance or KVM uses > another sequence of instructions for KVM prefix? I don't know if you've spotted, but the prefix is a ud2a instruction followed by 'xen' in ascii. The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > >> It might be better to generalise the decode support to "virtualisation >> escape prefix" or something slightly more generic. > Agreed, it is easy to expand it, we can switch the prefix template. > Could you tell me where I should look? I will add it. These are the only two I'm aware of. ~Andrew
On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote: > I don't know if you've spotted, but the prefix is a ud2a instruction > followed by 'xen' in ascii. > > The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 While the Xen one disassebles to valid instructions, that KVM one does not: .text xen: ud2; .ascii "xen" kvm: ud2; .ascii "kvm" disassembles like: 0000000000000000 <xen>: 0: 0f 0b ud2 2: 78 65 js 69 <kvm+0x64> 4: 6e outsb %ds:(%rsi),(%dx) 0000000000000005 <kvm>: 5: 0f 0b ud2 7: 6b .byte 0x6b 8: 76 6d jbe 77 <kvm+0x72> Which is a bit unfortunate I suppose. At least they don't appear to consume further bytes. I know it is water under the bridge at this point; but you could've used UD1 with a displacement with some 'unlikely' values. That way it would've decoded to a single instruction. Something like: ud1 0x6e6578(%rax),%rax which spells out "xen\0" in the displacement: 48 0f b9 80 78 65 6e 00
On 05/09/2019 09:26, Peter Zijlstra wrote: > On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote: > >> I don't know if you've spotted, but the prefix is a ud2a instruction >> followed by 'xen' in ascii. >> >> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > While the Xen one disassebles to valid instructions, that KVM one does > not: > > .text > xen: > ud2; .ascii "xen" > kvm: > ud2; .ascii "kvm" > > disassembles like: > > 0000000000000000 <xen>: > 0: 0f 0b ud2 > 2: 78 65 js 69 <kvm+0x64> > 4: 6e outsb %ds:(%rsi),(%dx) > 0000000000000005 <kvm>: > 5: 0f 0b ud2 > 7: 6b .byte 0x6b > 8: 76 6d jbe 77 <kvm+0x72> > > Which is a bit unfortunate I suppose. At least they don't appear to > consume further bytes. It does when you give objdump one extra byte to look at. 0000000000000005 <kvm>: 5: 0f 0b ud2 7: 6b 76 6d 00 imul $0x0,0x6d(%rsi),%esi I did try to point out that this property should have been checked before settling on 'kvm' as the string. As for the Xen prefix, it's introduction pre-dates me substantially, and I don't know whether the disassembly was considered, or we just got lucky. IMO, the string 'Xen' would would have been sightly nicer 0000000000000005 <Xen>: 5: 0f 0b ud2 7: 58 pop %rax 8: 65 6e outsb %gs:(%rsi),(%dx) but we're 13 years too late to amend this. > I know it is water under the bridge at this point; but you could've used > UD1 with a displacement with some 'unlikely' values. That way it > would've decoded to a single instruction. > > Something like: > > ud1 0x6e6578(%rax),%rax > > which spells out "xen\0" in the displacement: > > 48 0f b9 80 78 65 6e 00 :) I seem to recall UD0 and UD1 being very late to the documentation party. ~Andrew
On Thu, Sep 05, 2019 at 09:53:32AM +0100, Andrew Cooper wrote: > On 05/09/2019 09:26, Peter Zijlstra wrote: > > On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote: > > > >> I don't know if you've spotted, but the prefix is a ud2a instruction > >> followed by 'xen' in ascii. > >> > >> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > > While the Xen one disassebles to valid instructions, that KVM one does > > not: > > > > .text > > xen: > > ud2; .ascii "xen" > > kvm: > > ud2; .ascii "kvm" > > > > disassembles like: > > > > 0000000000000000 <xen>: > > 0: 0f 0b ud2 > > 2: 78 65 js 69 <kvm+0x64> > > 4: 6e outsb %ds:(%rsi),(%dx) > > 0000000000000005 <kvm>: > > 5: 0f 0b ud2 > > 7: 6b .byte 0x6b > > 8: 76 6d jbe 77 <kvm+0x72> > > > > Which is a bit unfortunate I suppose. At least they don't appear to > > consume further bytes. > > It does when you give objdump one extra byte to look at. > > 0000000000000005 <kvm>: > 5: 0f 0b ud2 > 7: 6b 76 6d 00 imul $0x0,0x6d(%rsi),%esi > > I did try to point out that this property should have been checked > before settling on 'kvm' as the string. Bah you're right; when I write: ud2; .ascii "kvm"; cpuid The output is gibberish :/ > but we're 13 years too late to amend this. Yah, I figured :/ > > I know it is water under the bridge at this point; but you could've used > > UD1 with a displacement with some 'unlikely' values. That way it > > would've decoded to a single instruction. > > > > Something like: > > > > ud1 0x6e6578(%rax),%rax > > > > which spells out "xen\0" in the displacement: > > > > 48 0f b9 80 78 65 6e 00 > > :) > > I seem to recall UD0 and UD1 being very late to the documentation party. They were; and the documentation for still UD0 differs between vendors :/
On 05/09/2019 10:26, Peter Zijlstra wrote: > On Thu, Sep 05, 2019 at 09:53:32AM +0100, Andrew Cooper wrote: >> On 05/09/2019 09:26, Peter Zijlstra wrote: >>> On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote: >>> >>>> I don't know if you've spotted, but the prefix is a ud2a instruction >>>> followed by 'xen' in ascii. >>>> >>>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 >>> While the Xen one disassebles to valid instructions, that KVM one does >>> not: >>> >>> .text >>> xen: >>> ud2; .ascii "xen" >>> kvm: >>> ud2; .ascii "kvm" >>> >>> disassembles like: >>> >>> 0000000000000000 <xen>: >>> 0: 0f 0b ud2 >>> 2: 78 65 js 69 <kvm+0x64> >>> 4: 6e outsb %ds:(%rsi),(%dx) >>> 0000000000000005 <kvm>: >>> 5: 0f 0b ud2 >>> 7: 6b .byte 0x6b >>> 8: 76 6d jbe 77 <kvm+0x72> >>> >>> Which is a bit unfortunate I suppose. At least they don't appear to >>> consume further bytes. >> It does when you give objdump one extra byte to look at. >> >> 0000000000000005 <kvm>: >> 5: 0f 0b ud2 >> 7: 6b 76 6d 00 imul $0x0,0x6d(%rsi),%esi >> >> I did try to point out that this property should have been checked >> before settling on 'kvm' as the string. > Bah you're right; when I write: > > ud2; .ascii "kvm"; cpuid > > The output is gibberish :/ KVM could possibly amend this. It is an off-by-default testing-only interface. Xen's is part of the ABI for ring-deprivielged guests, to force CPUID to be the virtualised view on hardware without CPUID Faulting. ~Andrew
On Thu, 5 Sep 2019 08:54:17 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 05/09/2019 02:49, Masami Hiramatsu wrote: > > On Wed, 4 Sep 2019 12:54:55 +0100 > > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > >> On 04/09/2019 12:45, Masami Hiramatsu wrote: > >>> Hi, > >>> > >>> These patches allow x86 instruction decoder to decode > >>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit > >>> kprobes to probe on it. > >>> > >>> Josh reported that the objtool can not decode such special > >>> prefixed instructions, and I found that we also have to > >>> prohibit kprobes to probe on such instruction. > >>> > >>> This series can be applied on -tip master branch which > >>> has merged Josh's objtool/perf sharing common x86 insn > >>> decoder series. > >> The paravirtualised xen-cpuid is were you'll see it most in a regular > >> kernel, but be aware that it is also used for testing purposes in other > >> circumstances, and there is an equivalent KVM prefix which is used for > >> KVM testing. > > Good catch! I didn't notice that. Is that really same sequance or KVM uses > > another sequence of instructions for KVM prefix? > > I don't know if you've spotted, but the prefix is a ud2a instruction > followed by 'xen' in ascii. > > The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > Ah, OK. I see it. But it seems that another ud0/ud1 can be used by other (new) virtualization. So at this moment I will just add a sequence as a pattern of prefix. Not use a fixed ud2 + sig. Thank you, > > > >> It might be better to generalise the decode support to "virtualisation > >> escape prefix" or something slightly more generic. > > Agreed, it is easy to expand it, we can switch the prefix template. > > Could you tell me where I should look? I will add it. > > These are the only two I'm aware of. > > ~Andrew
On Thu, 5 Sep 2019 09:53:32 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 05/09/2019 09:26, Peter Zijlstra wrote: > > On Thu, Sep 05, 2019 at 08:54:17AM +0100, Andrew Cooper wrote: > > > >> I don't know if you've spotted, but the prefix is a ud2a instruction > >> followed by 'xen' in ascii. > >> > >> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > > While the Xen one disassebles to valid instructions, that KVM one does > > not: > > > > .text > > xen: > > ud2; .ascii "xen" > > kvm: > > ud2; .ascii "kvm" > > > > disassembles like: > > > > 0000000000000000 <xen>: > > 0: 0f 0b ud2 > > 2: 78 65 js 69 <kvm+0x64> > > 4: 6e outsb %ds:(%rsi),(%dx) > > 0000000000000005 <kvm>: > > 5: 0f 0b ud2 > > 7: 6b .byte 0x6b > > 8: 76 6d jbe 77 <kvm+0x72> > > > > Which is a bit unfortunate I suppose. At least they don't appear to > > consume further bytes. > > It does when you give objdump one extra byte to look at. > > 0000000000000005 <kvm>: > 5: 0f 0b ud2 > 7: 6b 76 6d 00 imul $0x0,0x6d(%rsi),%esi > Hmm, that consumes the first byte of the next instruction. For example, .text xen: ud2; .ascii "xen"; cpuid kvm: ud2; .ascii "kvm"; cpuid 0000000000000000 <xen>: 0: 0f 0b ud2 2: 78 65 js 69 <kvm+0x62> 4: 6e outsb %ds:(%rsi),(%dx) 5: 0f a2 cpuid 0000000000000007 <kvm>: 7: 0f 0b ud2 9: 6b 76 6d 0f imul $0xf,0x6d(%rsi),%esi d: a2 .byte 0xa2 This will disturbe decoding bytestream. Anyway, with the next version it will be fixed in x86 insn decoder. Thanks,
On Thu, 5 Sep 2019 20:32:24 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Thu, 5 Sep 2019 08:54:17 +0100 > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > On 05/09/2019 02:49, Masami Hiramatsu wrote: > > > On Wed, 4 Sep 2019 12:54:55 +0100 > > > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > > >> On 04/09/2019 12:45, Masami Hiramatsu wrote: > > >>> Hi, > > >>> > > >>> These patches allow x86 instruction decoder to decode > > >>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit > > >>> kprobes to probe on it. > > >>> > > >>> Josh reported that the objtool can not decode such special > > >>> prefixed instructions, and I found that we also have to > > >>> prohibit kprobes to probe on such instruction. > > >>> > > >>> This series can be applied on -tip master branch which > > >>> has merged Josh's objtool/perf sharing common x86 insn > > >>> decoder series. > > >> The paravirtualised xen-cpuid is were you'll see it most in a regular > > >> kernel, but be aware that it is also used for testing purposes in other > > >> circumstances, and there is an equivalent KVM prefix which is used for > > >> KVM testing. > > > Good catch! I didn't notice that. Is that really same sequance or KVM uses > > > another sequence of instructions for KVM prefix? > > > > I don't know if you've spotted, but the prefix is a ud2a instruction > > followed by 'xen' in ascii. > > > > The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 Hmm, I think I might misunderstand what the "emulate prefix"... that is not a prefix which replace actual prefix, but just works like an escape sequence. Thus the next instruction can have any x86 prefix, correct? If so, this patch doesn't work. I have to add a new field in struct insn like "insn.emulate_prefix_size" so that we can keep a room for the prefixes for real instruction. Thank you,
On 05/09/2019 14:09, Masami Hiramatsu wrote: > On Thu, 5 Sep 2019 20:32:24 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > >> On Thu, 5 Sep 2019 08:54:17 +0100 >> Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >>> On 05/09/2019 02:49, Masami Hiramatsu wrote: >>>> On Wed, 4 Sep 2019 12:54:55 +0100 >>>> Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> >>>>> On 04/09/2019 12:45, Masami Hiramatsu wrote: >>>>>> Hi, >>>>>> >>>>>> These patches allow x86 instruction decoder to decode >>>>>> xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit >>>>>> kprobes to probe on it. >>>>>> >>>>>> Josh reported that the objtool can not decode such special >>>>>> prefixed instructions, and I found that we also have to >>>>>> prohibit kprobes to probe on such instruction. >>>>>> >>>>>> This series can be applied on -tip master branch which >>>>>> has merged Josh's objtool/perf sharing common x86 insn >>>>>> decoder series. >>>>> The paravirtualised xen-cpuid is were you'll see it most in a regular >>>>> kernel, but be aware that it is also used for testing purposes in other >>>>> circumstances, and there is an equivalent KVM prefix which is used for >>>>> KVM testing. >>>> Good catch! I didn't notice that. Is that really same sequance or KVM uses >>>> another sequence of instructions for KVM prefix? >>> I don't know if you've spotted, but the prefix is a ud2a instruction >>> followed by 'xen' in ascii. >>> >>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > Hmm, I think I might misunderstand what the "emulate prefix"... that is not > a prefix which replace actual prefix, but just works like an escape sequence. > Thus the next instruction can have any x86 prefix, correct? There is a bit of history here :) Originally, 13 years ago, Xen invented the "Force Emulate Prefix", which was the sequence: ud2a; .ascii 'xen'; cpuid which hit the #UD handler and was recognised as a request for virtualised CPUID information. This was for ring-deprivileged virtualisation, and is needed because the CPUID instruction itself doesn't trap to the hypervisor. Following some security issues in our instruction emulator, I reused this prefix with VT-x/SVM guests for testing purposes. It behaves in a similar manner - when enabled, it is recognised in #UD exception intercept, and causes Xen to add 5 to the instruction pointer, then emulate the instruction starting there. Then various folk thought that having the same kind of ability to test KVM's instruction emulator would be a good idea, so they borrowed the idea. From a behaviour point of view, it is an opaque 5 bytes which means "break into the hypervisor, then emulate the following instruction". The name "prefix" is unfortunate. It was named thusly because from the programmers point of view, it was something you put before the CPUID instruction which wanted to be emulated. It is not related to x86 instruction concept of a prefix. ~Andrew
On Thu, 5 Sep 2019 14:31:56 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >>> The KVM version was added in c/s 6c86eedc206dd1f9d37a2796faa8e6f2278215d2 > > Hmm, I think I might misunderstand what the "emulate prefix"... that is not > > a prefix which replace actual prefix, but just works like an escape sequence. > > Thus the next instruction can have any x86 prefix, correct? > > There is a bit of history here :) > > Originally, 13 years ago, Xen invented the "Force Emulate Prefix", which > was the sequence: > > ud2a; .ascii 'xen'; cpuid > > which hit the #UD handler and was recognised as a request for > virtualised CPUID information. This was for ring-deprivileged > virtualisation, and is needed because the CPUID instruction itself > doesn't trap to the hypervisor. > > Following some security issues in our instruction emulator, I reused > this prefix with VT-x/SVM guests for testing purposes. It behaves in a > similar manner - when enabled, it is recognised in #UD exception > intercept, and causes Xen to add 5 to the instruction pointer, then > emulate the instruction starting there. > > Then various folk thought that having the same kind of ability to test > KVM's instruction emulator would be a good idea, so they borrowed the idea. > > From a behaviour point of view, it is an opaque 5 bytes which means > "break into the hypervisor, then emulate the following instruction". > > The name "prefix" is unfortunate. It was named thusly because from the > programmers point of view, it was something you put before the CPUID > instruction which wanted to be emulated. It is not related to x86 > instruction concept of a prefix. OK, then we should not use the insn->prefixes for those escape sequences. Thank you,
Hi, These patches allow x86 instruction decoder to decode xen-cpuid which has XEN_EMULATE_PREFIX, and prohibit kprobes to probe on it. Josh reported that the objtool can not decode such special prefixed instructions, and I found that we also have to prohibit kprobes to probe on such instruction. This series can be applied on -tip master branch which has merged Josh's objtool/perf sharing common x86 insn decoder series. Thank you, --- Masami Hiramatsu (2): x86: xen: insn: Decode XEN_EMULATE_PREFIX correctly x86: kprobes: Prohibit probing on instruction which has Xen prefix arch/x86/include/asm/insn.h | 2 + arch/x86/include/asm/xen/interface.h | 7 ++++- arch/x86/include/asm/xen/prefix.h | 10 +++++++ arch/x86/kernel/kprobes/core.c | 4 +++ arch/x86/lib/insn.c | 43 +++++++++++++++++++++++++++++++ tools/arch/x86/include/asm/insn.h | 2 + tools/arch/x86/include/asm/xen/prefix.h | 10 +++++++ tools/arch/x86/lib/insn.c | 43 +++++++++++++++++++++++++++++++ tools/objtool/sync-check.sh | 3 +- 9 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 arch/x86/include/asm/xen/prefix.h create mode 100644 tools/arch/x86/include/asm/xen/prefix.h -- Masami Hiramatsu (Linaro) <mhiramat@kernel.org>