| 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: <Ys5zgem4Zp7y7ajh@kroah.com>
Date: Wed, 13 Jul 2022 09:25:53 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: linux-kernel@...r.kernel.org, stable@...r.kernel.org,
Borislav Petkov <bp@...e.de>,
Masami Hiramatsu <mhiramat@...nel.org>,
Ben Hutchings <ben@...adent.org.uk>
Subject: Re: [PATCH 5.10 017/130] x86/insn: Add an insn_decode() API
On Tue, Jul 12, 2022 at 04:04:29PM -0700, Florian Fainelli wrote:
> On 7/12/22 11:37, Greg Kroah-Hartman wrote:
> > From: Borislav Petkov <bp@...e.de>
> >
> > commit 93281c4a96572a34504244969b938e035204778d upstream.
> >
> > Users of the instruction decoder should use this to decode instruction
> > bytes. For that, have insn*() helpers return an int value to denote
> > success/failure. When there's an error fetching the next insn byte and
> > the insn falls short, return -ENODATA to denote that.
> >
> > While at it, make insn_get_opcode() more stricter as to whether what has
> > seen so far is a valid insn and if not.
> >
> > Copy linux/kconfig.h for the tools-version of the decoder so that it can
> > use IS_ENABLED().
> >
> > Also, cast the INSN_MODE_KERN dummy define value to (enum insn_mode)
> > for tools use of the decoder because perf tool builds with -Werror and
> > errors out with -Werror=sign-compare otherwise.
> >
> > Signed-off-by: Borislav Petkov <bp@...e.de>
> > Acked-by: Masami Hiramatsu <mhiramat@...nel.org>
> > Link: https://lkml.kernel.org/r/20210304174237.31945-5-bp@alien8.de
> > Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > ---
> > arch/x86/include/asm/insn.h | 24 ++--
> > arch/x86/lib/insn.c | 216 +++++++++++++++++++++++++++++-------
> > tools/arch/x86/include/asm/insn.h | 24 ++--
> > tools/arch/x86/lib/insn.c | 222 +++++++++++++++++++++++++++++---------
> > tools/include/linux/kconfig.h | 73 ++++++++++++
> > 5 files changed, 452 insertions(+), 107 deletions(-)
> > create mode 100644 tools/include/linux/kconfig.h
> >
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -87,13 +87,23 @@ struct insn {
> > #define X86_VEX_M_MAX 0x1f /* VEX3.M Maximum value */
> > 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);
> > -extern void insn_get_sib(struct insn *insn);
> > -extern void insn_get_displacement(struct insn *insn);
> > -extern void insn_get_immediate(struct insn *insn);
> > -extern void insn_get_length(struct insn *insn);
> > +extern int insn_get_prefixes(struct insn *insn);
> > +extern int insn_get_opcode(struct insn *insn);
> > +extern int insn_get_modrm(struct insn *insn);
> > +extern int insn_get_sib(struct insn *insn);
> > +extern int insn_get_displacement(struct insn *insn);
> > +extern int insn_get_immediate(struct insn *insn);
> > +extern int insn_get_length(struct insn *insn);
> > +
> > +enum insn_mode {
> > + INSN_MODE_32,
> > + INSN_MODE_64,
> > + /* Mode is determined by the current kernel build. */
> > + INSN_MODE_KERN,
> > + INSN_NUM_MODES,
> > +};
> > +
> > +extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
> > /* Attribute will be determined after getting ModRM (for opcode groups) */
> > static inline void insn_get_attribute(struct insn *insn)
> > --- a/arch/x86/lib/insn.c
> > +++ b/arch/x86/lib/insn.c
> > @@ -13,6 +13,9 @@
> > #include <asm/inat.h> /*__ignore_sync_check__ */
> > #include <asm/insn.h> /* __ignore_sync_check__ */
> > +#include <linux/errno.h>
> > +#include <linux/kconfig.h>
> > +
> > #include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
> > /* Verify next sizeof(t) bytes can be on the same instruction */
> > @@ -97,8 +100,12 @@ static void insn_get_emulate_prefix(stru
> > * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
> > * to point to the (first) opcode. No effect if @insn->prefixes.got
> > * is already set.
> > + *
> > + * * Returns:
> > + * 0: on success
> > + * < 0: on error
> > */
> > -void insn_get_prefixes(struct insn *insn)
> > +int insn_get_prefixes(struct insn *insn)
> > {
> > struct insn_field *prefixes = &insn->prefixes;
> > insn_attr_t attr;
> > @@ -106,7 +113,7 @@ void insn_get_prefixes(struct insn *insn
> > int i, nb;
> > if (prefixes->got)
> > - return;
> > + return 0;
> > insn_get_emulate_prefix(insn);
> > @@ -217,8 +224,10 @@ vex_end:
> > prefixes->got = 1;
> > + return 0;
> > +
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > /**
> > @@ -230,16 +239,25 @@ err_out:
> > * If necessary, first collects any preceding (prefix) bytes.
> > * Sets @insn->opcode.value = opcode1. No effect if @insn->opcode.got
> > * is already 1.
> > + *
> > + * Returns:
> > + * 0: on success
> > + * < 0: on error
> > */
> > -void insn_get_opcode(struct insn *insn)
> > +int insn_get_opcode(struct insn *insn)
> > {
> > struct insn_field *opcode = &insn->opcode;
> > + int pfx_id, ret;
> > insn_byte_t op;
> > - int pfx_id;
> > +
> > if (opcode->got)
> > - return;
> > - if (!insn->prefixes.got)
> > - insn_get_prefixes(insn);
> > + return 0;
> > +
> > + if (!insn->prefixes.got) {
> > + ret = insn_get_prefixes(insn);
> > + if (ret)
> > + return ret;
> > + }
> > /* Get first opcode */
> > op = get_next(insn_byte_t, insn);
> > @@ -254,9 +272,13 @@ void insn_get_opcode(struct insn *insn)
> > insn->attr = inat_get_avx_attribute(op, m, p);
> > if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
> > (!inat_accept_vex(insn->attr) &&
> > - !inat_is_group(insn->attr)))
> > - insn->attr = 0; /* This instruction is bad */
> > - goto end; /* VEX has only 1 byte for opcode */
> > + !inat_is_group(insn->attr))) {
> > + /* This instruction is bad */
> > + insn->attr = 0;
> > + return -EINVAL;
> > + }
> > + /* VEX has only 1 byte for opcode */
> > + goto end;
> > }
> > insn->attr = inat_get_opcode_attribute(op);
> > @@ -267,13 +289,18 @@ void insn_get_opcode(struct insn *insn)
> > pfx_id = insn_last_prefix_id(insn);
> > insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr);
> > }
> > - if (inat_must_vex(insn->attr))
> > - insn->attr = 0; /* This instruction is bad */
> > +
> > + if (inat_must_vex(insn->attr)) {
> > + /* This instruction is bad */
> > + insn->attr = 0;
> > + return -EINVAL;
> > + }
> > end:
> > opcode->got = 1;
> > + return 0;
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > /**
> > @@ -283,15 +310,25 @@ err_out:
> > * Populates @insn->modrm and updates @insn->next_byte to point past the
> > * ModRM byte, if any. If necessary, first collects the preceding bytes
> > * (prefixes and opcode(s)). No effect if @insn->modrm.got is already 1.
> > + *
> > + * Returns:
> > + * 0: on success
> > + * < 0: on error
> > */
> > -void insn_get_modrm(struct insn *insn)
> > +int insn_get_modrm(struct insn *insn)
> > {
> > struct insn_field *modrm = &insn->modrm;
> > insn_byte_t pfx_id, mod;
> > + int ret;
> > +
> > if (modrm->got)
> > - return;
> > - if (!insn->opcode.got)
> > - insn_get_opcode(insn);
> > + return 0;
> > +
> > + if (!insn->opcode.got) {
> > + ret = insn_get_opcode(insn);
> > + if (ret)
> > + return ret;
> > + }
> > if (inat_has_modrm(insn->attr)) {
> > mod = get_next(insn_byte_t, insn);
> > @@ -301,17 +338,22 @@ void insn_get_modrm(struct insn *insn)
> > pfx_id = insn_last_prefix_id(insn);
> > insn->attr = inat_get_group_attribute(mod, pfx_id,
> > insn->attr);
> > - if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
> > - insn->attr = 0; /* This is bad */
> > + if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
> > + /* Bad insn */
> > + insn->attr = 0;
> > + return -EINVAL;
> > + }
> > }
> > }
> > if (insn->x86_64 && inat_is_force64(insn->attr))
> > insn->opnd_bytes = 8;
> > +
> > modrm->got = 1;
> > + return 0;
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > @@ -325,11 +367,16 @@ err_out:
> > int insn_rip_relative(struct insn *insn)
> > {
> > struct insn_field *modrm = &insn->modrm;
> > + int ret;
> > if (!insn->x86_64)
> > return 0;
> > - if (!modrm->got)
> > - insn_get_modrm(insn);
> > +
> > + if (!modrm->got) {
> > + ret = insn_get_modrm(insn);
> > + if (ret)
> > + return 0;
> > + }
> > /*
> > * For rip-relative instructions, the mod field (top 2 bits)
> > * is zero and the r/m field (bottom 3 bits) is 0x5.
> > @@ -343,15 +390,25 @@ int insn_rip_relative(struct insn *insn)
> > *
> > * If necessary, first collects the instruction up to and including the
> > * ModRM byte.
> > + *
> > + * Returns:
> > + * 0: if decoding succeeded
> > + * < 0: otherwise.
> > */
> > -void insn_get_sib(struct insn *insn)
> > +int insn_get_sib(struct insn *insn)
> > {
> > insn_byte_t modrm;
> > + int ret;
> > if (insn->sib.got)
> > - return;
> > - if (!insn->modrm.got)
> > - insn_get_modrm(insn);
> > + return 0;
> > +
> > + if (!insn->modrm.got) {
> > + ret = insn_get_modrm(insn);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > if (insn->modrm.nbytes) {
> > modrm = (insn_byte_t)insn->modrm.value;
> > if (insn->addr_bytes != 2 &&
> > @@ -362,8 +419,10 @@ void insn_get_sib(struct insn *insn)
> > }
> > insn->sib.got = 1;
> > + return 0;
> > +
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > @@ -374,15 +433,25 @@ err_out:
> > * If necessary, first collects the instruction up to and including the
> > * SIB byte.
> > * Displacement value is sign-expanded.
> > + *
> > + * * Returns:
> > + * 0: if decoding succeeded
> > + * < 0: otherwise.
> > */
> > -void insn_get_displacement(struct insn *insn)
> > +int insn_get_displacement(struct insn *insn)
> > {
> > insn_byte_t mod, rm, base;
> > + int ret;
> > if (insn->displacement.got)
> > - return;
> > - if (!insn->sib.got)
> > - insn_get_sib(insn);
> > + return 0;
> > +
> > + if (!insn->sib.got) {
> > + ret = insn_get_sib(insn);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > if (insn->modrm.nbytes) {
> > /*
> > * Interpreting the modrm byte:
> > @@ -425,9 +494,10 @@ void insn_get_displacement(struct insn *
> > }
> > out:
> > insn->displacement.got = 1;
> > + return 0;
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > /* Decode moffset16/32/64. Return 0 if failed */
> > @@ -538,20 +608,30 @@ err_out:
> > }
> > /**
> > - * insn_get_immediate() - Get the immediates of instruction
> > + * insn_get_immediate() - Get the immediate in an instruction
> > * @insn: &struct insn containing instruction
> > *
> > * If necessary, first collects the instruction up to and including the
> > * displacement bytes.
> > * Basically, most of immediates are sign-expanded. Unsigned-value can be
> > - * get by bit masking with ((1 << (nbytes * 8)) - 1)
> > + * computed by bit masking with ((1 << (nbytes * 8)) - 1)
> > + *
> > + * Returns:
> > + * 0: on success
> > + * < 0: on error
> > */
> > -void insn_get_immediate(struct insn *insn)
> > +int insn_get_immediate(struct insn *insn)
> > {
> > + int ret;
> > +
> > if (insn->immediate.got)
> > - return;
> > - if (!insn->displacement.got)
> > - insn_get_displacement(insn);
> > + return 0;
> > +
> > + if (!insn->displacement.got) {
> > + ret = insn_get_displacement(insn);
> > + if (ret)
> > + return ret;
> > + }
> > if (inat_has_moffset(insn->attr)) {
> > if (!__get_moffset(insn))
> > @@ -604,9 +684,10 @@ void insn_get_immediate(struct insn *ins
> > }
> > done:
> > insn->immediate.got = 1;
> > + return 0;
> > err_out:
> > - return;
> > + return -ENODATA;
> > }
> > /**
> > @@ -615,13 +696,58 @@ err_out:
> > *
> > * If necessary, first collects the instruction up to and including the
> > * immediates bytes.
> > - */
> > -void insn_get_length(struct insn *insn)
> > + *
> > + * Returns:
> > + * - 0 on success
> > + * - < 0 on error
> > +*/
> > +int insn_get_length(struct insn *insn)
> > {
> > + int ret;
> > +
> > if (insn->length)
> > - return;
> > - if (!insn->immediate.got)
> > - insn_get_immediate(insn);
> > + return 0;
> > +
> > + if (!insn->immediate.got) {
> > + ret = insn_get_immediate(insn);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > insn->length = (unsigned char)((unsigned long)insn->next_byte
> > - (unsigned long)insn->kaddr);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * insn_decode() - Decode an x86 instruction
> > + * @insn: &struct insn to be initialized
> > + * @kaddr: address (in kernel memory) of instruction (or copy thereof)
> > + * @buf_len: length of the insn buffer at @kaddr
> > + * @m: insn mode, see enum insn_mode
> > + *
> > + * Returns:
> > + * 0: if decoding succeeded
> > + * < 0: otherwise.
> > + */
> > +int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
> > +{
> > + int ret;
> > +
> > +/* #define INSN_MODE_KERN -1 __ignore_sync_check__ mode is only valid in the kernel */
> > +
> > + if (m == INSN_MODE_KERN)
> > + insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
> > + else
> > + insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
> > +
> > + ret = insn_get_length(insn);
> > + if (ret)
> > + return ret;
> > +
> > + if (insn_complete(insn))
> > + return 0;
> > +
> > + return -EINVAL;
> > }
> > --- a/tools/arch/x86/include/asm/insn.h
> > +++ b/tools/arch/x86/include/asm/insn.h
> > @@ -87,13 +87,23 @@ struct insn {
> > #define X86_VEX_M_MAX 0x1f /* VEX3.M Maximum value */
> > 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);
> > -extern void insn_get_sib(struct insn *insn);
> > -extern void insn_get_displacement(struct insn *insn);
> > -extern void insn_get_immediate(struct insn *insn);
> > -extern void insn_get_length(struct insn *insn);
> > +extern int insn_get_prefixes(struct insn *insn);
> > +extern int insn_get_opcode(struct insn *insn);
> > +extern int insn_get_modrm(struct insn *insn);
> > +extern int insn_get_sib(struct insn *insn);
> > +extern int insn_get_displacement(struct insn *insn);
> > +extern int insn_get_immediate(struct insn *insn);
> > +extern int insn_get_length(struct insn *insn);
> > +
> > +enum insn_mode {
> > + INSN_MODE_32,
> > + INSN_MODE_64,
> > + /* Mode is determined by the current kernel build. */
> > + INSN_MODE_KERN,
> > + INSN_NUM_MODES,
> > +};
> > +
> > +extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
> > /* Attribute will be determined after getting ModRM (for opcode groups) */
> > static inline void insn_get_attribute(struct insn *insn)
> > --- a/tools/arch/x86/lib/insn.c
> > +++ b/tools/arch/x86/lib/insn.c
> > @@ -10,10 +10,13 @@
> > #else
> > #include <string.h>
> > #endif
> > -#include "../include/asm/inat.h" /* __ignore_sync_check__ */
> > -#include "../include/asm/insn.h" /* __ignore_sync_check__ */
> > +#include <asm/inat.h> /* __ignore_sync_check__ */
> > +#include <asm/insn.h> /* __ignore_sync_check__ */
>
> These includes breaks the build for me with:
>
> CC /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.o
> In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15:
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c:13:10: fatal error:
> asm/inat.h: No such file or directory
> #include <asm/inat.h> /* __ignore_sync_check__ */
> ^~~~~~~~~~~~
> compilation terminated.
> make[7]: *** [util/intel-pt-decoder/Build:14: /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.o]
> Error 1
> make[6]: *** [/local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/build/Makefile.build:139:
> intel-pt-decoder] Error 2
> make[5]: *** [/local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/build/Makefile.build:139:
> util] Error 2
> make[4]: *** [Makefile.perf:643: /local/users/fainelli/buildroot/output/arm64/build/linux-custom/tools/perf/perf-in.o]
> Error 2
> make[3]: *** [Makefile.perf:229: sub-make] Error 2
> make[2]: *** [Makefile:70: all] Error 2
> make[1]: *** [package/pkg-generic.mk:294:
> /local/users/fainelli/buildroot/output/arm64/build/linux-tools/.stamp_built]
> Error 2
> make: *** [Makefile:27: _all] Error 2
>
> It looks like you would also need to back port this:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0705ef64d1ff52b817e278ca6e28095585ff31e1
Thanks for this, now queued up.
greg k-h
Powered by blists - more mailing lists