| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5464C4FD.60705@hitachi.com>
Date: Thu, 13 Nov 2014 23:49:33 +0900
From: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To: Dave Hansen <dave@...1.net>
Cc: linux-kernel@...r.kernel.org, dave.hansen@...ux.intel.com,
x86@...nel.org, a.p.zijlstra@...llo.nl, paulus@...ba.org,
acme@...nel.org, jkenisto@...ibm.com, srikar@...ux.vnet.ibm.com,
tglx@...utronix.de, ananth@...ibm.com,
anil.s.keshavamurthy@...el.com, davem@...emloft.net
Subject: Re: [PATCH] x86: remove arbitrary instruction size limit in instruction
decoder
(2014/11/13 7:53), Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@...ux.intel.com>
>
> The current x86 instruction decoder steps along through the
> instruction stream but always ensures that it never steps farther
> than the largest possible instruction size (MAX_INSN_SIZE).
>
> The MPX code is now going to be doing some decoding of userspace
> instructions. We copy those from userspace in to the kernel and
> they're obviously completely untrusted coming from userspace. In
> addition to the constraint that instructions can only be so long,
> we also have to be aware of how long the buffer is that came in
> from userspace. This _looks_ to be similar to what the perf and
> kprobes is doing, but it's unclear to me whether they are
> affected.
>
> We shouldn't simply error out when we get short copy_from_user*()
> results from userspace (like intel_pmu_pebs_fixup_ip() does
> currently). It is perfectly valid to be executing an instruction
> within MAX_INSN_SIZE bytes of an unreadable page. We should be
> able to gracefully handle short reads in those cases.
>
> This adds support to the decoder to record how long the buffer
> being decoded is and to refuse to "validate" the instruction if
> we would have gone over the end of the buffer to decode it.
>
> The kprobes code probably needs to be looked at here a bit more
> carefully. This patch still respects the MAX_INSN_SIZE limit
> there but the kprobes code does look like it might be able to
> be a bit more strict than it currently is.
Would you mean kprobes can copy shorter? Maybe, but I think current
one is enough because it is on a cold path.
OK, at least this looks good to me.
>
> Note: the v10 version of the MPX patches I just posted depends
> on this patch.
BTW, current insn decoder doesn't support MPX... That should be
updated (add bnd* to x86-insn-map.txt)
Acked-by: Masami Hiramatsu <masmai.hiramatsu.pt@...achi.com>
>
> Signed-off-by: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: x86@...nel.org
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> Cc: Jim Keniston <jkenisto@...ibm.com>
> Cc: Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ananth N Mavinakayanahalli <ananth@...ibm.com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> ---
>
> b/arch/x86/include/asm/insn.h | 10 ++++++----
> b/arch/x86/kernel/cpu/perf_event_intel_ds.c | 9 ++++++---
> b/arch/x86/kernel/cpu/perf_event_intel_lbr.c | 2 +-
> b/arch/x86/kernel/kprobes/core.c | 8 +++++---
> b/arch/x86/kernel/kprobes/opt.c | 4 +++-
> b/arch/x86/kernel/uprobes.c | 2 +-
> b/arch/x86/lib/insn.c | 5 +++--
> b/arch/x86/tools/insn_sanity.c | 2 +-
> b/arch/x86/tools/test_get_len.c | 2 +-
> 9 files changed, 27 insertions(+), 17 deletions(-)
>
> diff -puN arch/x86/include/asm/insn.h~x86-insn-decoder-remove-arbitrary-limit arch/x86/include/asm/insn.h
> --- a/arch/x86/include/asm/insn.h~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.952753062 -0800
> +++ b/arch/x86/include/asm/insn.h 2014-11-12 12:45:52.969753829 -0800
> @@ -65,6 +65,7 @@ struct insn {
> unsigned char x86_64;
>
> const insn_byte_t *kaddr; /* kernel address of insn to analyze */
> + const insn_byte_t *end_kaddr; /* kernel address of last insn in buffer */
> const insn_byte_t *next_byte;
> };
>
> @@ -96,7 +97,7 @@ struct insn {
> #define X86_VEX_P(vex) ((vex) & 0x03) /* VEX3 Byte2, VEX2 Byte1 */
> #define X86_VEX_M_MAX 0x1f /* VEX3.M Maximum value */
>
> -extern void insn_init(struct insn *insn, const void *kaddr, int x86_64);
> +extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
> extern void insn_get_prefixes(struct insn *insn);
> extern void insn_get_opcode(struct insn *insn);
> extern void insn_get_modrm(struct insn *insn);
> @@ -115,12 +116,13 @@ static inline void insn_get_attribute(st
> extern int insn_rip_relative(struct insn *insn);
>
> /* Init insn for kernel text */
> -static inline void kernel_insn_init(struct insn *insn, const void *kaddr)
> +static inline void kernel_insn_init(struct insn *insn,
> + const void *kaddr, int buf_len)
> {
> #ifdef CONFIG_X86_64
> - insn_init(insn, kaddr, 1);
> + insn_init(insn, kaddr, buf_len, 1);
> #else /* CONFIG_X86_32 */
> - insn_init(insn, kaddr, 0);
> + insn_init(insn, kaddr, buf_len, 0);
> #endif
> }
>
> diff -puN arch/x86/kernel/cpu/perf_event_intel_ds.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/kernel/cpu/perf_event_intel_ds.c
> --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.954753152 -0800
> +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c 2014-11-12 12:45:52.970753874 -0800
> @@ -724,6 +724,7 @@ static int intel_pmu_pebs_fixup_ip(struc
> unsigned long ip = regs->ip;
> int is_64bit = 0;
> void *kaddr;
> + int size;
>
> /*
> * We don't need to fixup if the PEBS assist is fault like
> @@ -758,11 +759,12 @@ static int intel_pmu_pebs_fixup_ip(struc
> return 1;
> }
>
> + size = ip - to;
> if (!kernel_ip(ip)) {
> - int size, bytes;
> + int bytes;
> u8 *buf = this_cpu_read(insn_buffer);
>
> - size = ip - to; /* Must fit our buffer, see above */
> + /* 'size' must fit our buffer, see above */
> bytes = copy_from_user_nmi(buf, (void __user *)to, size);
> if (bytes != 0)
> return 0;
> @@ -780,11 +782,12 @@ static int intel_pmu_pebs_fixup_ip(struc
> #ifdef CONFIG_X86_64
> is_64bit = kernel_ip(to) || !test_thread_flag(TIF_IA32);
> #endif
> - insn_init(&insn, kaddr, is_64bit);
> + insn_init(&insn, kaddr, size, is_64bit);
> insn_get_length(&insn);
>
> to += insn.length;
> kaddr += insn.length;
> + size -= insn.length;
> } while (to < ip);
>
> if (to == ip) {
> diff -puN arch/x86/kernel/cpu/perf_event_intel_lbr.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/kernel/cpu/perf_event_intel_lbr.c
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.956753243 -0800
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c 2014-11-12 12:45:52.970753874 -0800
> @@ -518,7 +518,7 @@ static int branch_type(unsigned long fro
> #ifdef CONFIG_X86_64
> is64 = kernel_ip((unsigned long)addr) || !test_thread_flag(TIF_IA32);
> #endif
> - insn_init(&insn, addr, is64);
> + insn_init(&insn, addr, size, is64);
> insn_get_opcode(&insn);
>
> switch (insn.opcode.bytes[0]) {
> diff -puN arch/x86/kernel/kprobes/core.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/kernel/kprobes/core.c
> --- a/arch/x86/kernel/kprobes/core.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.957753288 -0800
> +++ b/arch/x86/kernel/kprobes/core.c 2014-11-12 12:45:52.970753874 -0800
> @@ -285,7 +285,7 @@ static int can_probe(unsigned long paddr
> * normally used, we just go through if there is no kprobe.
> */
> __addr = recover_probed_instruction(buf, addr);
> - kernel_insn_init(&insn, (void *)__addr);
> + kernel_insn_init(&insn, (void *)__addr, MAX_INSN_SIZE);
> insn_get_length(&insn);
>
> /*
> @@ -330,8 +330,10 @@ int __copy_instruction(u8 *dest, u8 *src
> {
> struct insn insn;
> kprobe_opcode_t buf[MAX_INSN_SIZE];
> + unsigned long recovered_insn =
> + recover_probed_instruction(buf, (unsigned long)src);
>
> - kernel_insn_init(&insn, (void *)recover_probed_instruction(buf, (unsigned long)src));
> + kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
> insn_get_length(&insn);
> /* Another subsystem puts a breakpoint, failed to recover */
> if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
> @@ -342,7 +344,7 @@ int __copy_instruction(u8 *dest, u8 *src
> if (insn_rip_relative(&insn)) {
> s64 newdisp;
> u8 *disp;
> - kernel_insn_init(&insn, dest);
> + kernel_insn_init(&insn, dest, insn.length);
> insn_get_displacement(&insn);
> /*
> * The copied instruction uses the %rip-relative addressing
> diff -puN arch/x86/kernel/kprobes/opt.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/kernel/kprobes/opt.c
> --- a/arch/x86/kernel/kprobes/opt.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.959753378 -0800
> +++ b/arch/x86/kernel/kprobes/opt.c 2014-11-12 12:45:52.971753919 -0800
> @@ -251,13 +251,15 @@ static int can_optimize(unsigned long pa
> /* Decode instructions */
> addr = paddr - offset;
> while (addr < paddr - offset + size) { /* Decode until function end */
> + unsigned long recovered_insn;
> if (search_exception_tables(addr))
> /*
> * Since some fixup code will jumps into this function,
> * we can't optimize kprobe in this function.
> */
> return 0;
> - kernel_insn_init(&insn, (void *)recover_probed_instruction(buf, addr));
> + recovered_insn = recover_probed_instruction(buf, addr);
> + kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
> insn_get_length(&insn);
> /* Another subsystem puts a breakpoint */
> if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
> diff -puN arch/x86/kernel/uprobes.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/kernel/uprobes.c
> --- a/arch/x86/kernel/uprobes.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.961753468 -0800
> +++ b/arch/x86/kernel/uprobes.c 2014-11-12 12:45:52.971753919 -0800
> @@ -219,7 +219,7 @@ static int uprobe_init_insn(struct arch_
> {
> u32 volatile *good_insns;
>
> - insn_init(insn, auprobe->insn, x86_64);
> + insn_init(insn, auprobe->insn, sizeof(auprobe->insn), x86_64);
> /* has the side-effect of processing the entire instruction */
> insn_get_length(insn);
> if (WARN_ON_ONCE(!insn_complete(insn)))
> diff -puN arch/x86/lib/insn.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/lib/insn.c
> --- a/arch/x86/lib/insn.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.962753513 -0800
> +++ b/arch/x86/lib/insn.c 2014-11-12 12:45:52.972753964 -0800
> @@ -28,7 +28,7 @@
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> - ((insn)->next_byte + sizeof(t) + n - (insn)->kaddr <= MAX_INSN_SIZE)
> + ((insn)->next_byte + sizeof(t) + n < (insn)->end_kaddr)
>
> #define __get_next(t, insn) \
> ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> @@ -50,10 +50,11 @@
> * @kaddr: address (in kernel memory) of instruction (or copy thereof)
> * @x86_64: !0 for 64-bit kernel or 64-bit app
> */
> -void insn_init(struct insn *insn, const void *kaddr, int x86_64)
> +void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64)
> {
> memset(insn, 0, sizeof(*insn));
> insn->kaddr = kaddr;
> + insn->end_kaddr = kaddr + buf_len;
> insn->next_byte = kaddr;
> insn->x86_64 = x86_64 ? 1 : 0;
> insn->opnd_bytes = 4;
> diff -puN arch/x86/tools/insn_sanity.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/tools/insn_sanity.c
> --- a/arch/x86/tools/insn_sanity.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.964753604 -0800
> +++ b/arch/x86/tools/insn_sanity.c 2014-11-12 12:45:52.972753964 -0800
> @@ -254,7 +254,7 @@ int main(int argc, char **argv)
> continue;
>
> /* Decode an instruction */
> - insn_init(&insn, insn_buf, x86_64);
> + insn_init(&insn, insn_buf, sizeof(insn_buf), x86_64);
> insn_get_length(&insn);
>
> if (insn.next_byte <= insn.kaddr ||
> diff -puN arch/x86/tools/test_get_len.c~x86-insn-decoder-remove-arbitrary-limit arch/x86/tools/test_get_len.c
> --- a/arch/x86/tools/test_get_len.c~x86-insn-decoder-remove-arbitrary-limit 2014-11-12 12:45:52.966753694 -0800
> +++ b/arch/x86/tools/test_get_len.c 2014-11-12 12:45:52.972753964 -0800
> @@ -149,7 +149,7 @@ int main(int argc, char **argv)
> break;
> }
> /* Decode an instruction */
> - insn_init(&insn, insn_buf, x86_64);
> + insn_init(&insn, insn_buf, sizeof(insn_buf), x86_64);
> insn_get_length(&insn);
> if (insn.length != nb) {
> warnings++;
> _
>
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists