* x86/insn-eval: negative return value?
@ 2017-11-22 20:45 Kees Cook
2017-11-22 21:22 ` Borislav Petkov
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-11-22 20:45 UTC (permalink / raw)
To: Ricardo Neri; +Cc: LKML, Borislav Petkov
Hi,
While doing some Clang test builds, this was reported:
arch/x86/lib/insn-eval.c:780:10: warning: implicit conversion from
'int' to 'char' changes value from 132 to -124 [-Wconstant-conversion]
return INSN_CODE_SEG_PARAMS(4, 8);
~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~
./arch/x86/include/asm/insn-eval.h:16:57: note: expanded from macro
'INSN_CODE_SEG_PARAMS'
#define INSN_CODE_SEG_PARAMS(oper_sz, addr_sz) (oper_sz | (addr_sz << 4))
~~~~~~~~^~~~~~~~~~~~~~~~
Is this really expected to wrap negative on IA-32e 64-bit mode case?
Thanks!
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86/insn-eval: negative return value? 2017-11-22 20:45 x86/insn-eval: negative return value? Kees Cook @ 2017-11-22 21:22 ` Borislav Petkov 2017-11-22 21:48 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Borislav Petkov @ 2017-11-22 21:22 UTC (permalink / raw) To: Kees Cook; +Cc: Ricardo Neri, LKML On Wed, Nov 22, 2017 at 12:45:17PM -0800, Kees Cook wrote: > Hi, > > While doing some Clang test builds, this was reported: > > arch/x86/lib/insn-eval.c:780:10: warning: implicit conversion from > 'int' to 'char' changes value from 132 to -124 [-Wconstant-conversion] > return INSN_CODE_SEG_PARAMS(4, 8); > ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~ > ./arch/x86/include/asm/insn-eval.h:16:57: note: expanded from macro > 'INSN_CODE_SEG_PARAMS' > #define INSN_CODE_SEG_PARAMS(oper_sz, addr_sz) (oper_sz | (addr_sz << 4)) > ~~~~~~~~^~~~~~~~~~~~~~~~ > > Is this really expected to wrap negative on IA-32e 64-bit mode case? Well, we were saving on structs and crammed two values into one but now that I look at that function, it returns -EINVAL too, which is an int, so its retval should simply be an int. IOW, I guess something like this: --- diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h index e1d3b4ce8a92..2b6ccf2c49f1 100644 --- a/arch/x86/include/asm/insn-eval.h +++ b/arch/x86/include/asm/insn-eval.h @@ -18,6 +18,6 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs); unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx); -char insn_get_code_seg_params(struct pt_regs *regs); +int insn_get_code_seg_params(struct pt_regs *regs); #endif /* _ASM_X86_INSN_EVAL_H */ diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c index dabbac30acdf..f44ce0fb3583 100644 --- a/arch/x86/kernel/umip.c +++ b/arch/x86/kernel/umip.c @@ -319,7 +319,7 @@ bool fixup_umip_exception(struct pt_regs *regs) unsigned char buf[MAX_INSN_SIZE]; void __user *uaddr; struct insn insn; - char seg_defs; + int seg_defs; if (!regs) return false; diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index 35625d279458..9119d8e41f1f 100644 --- a/arch/x86/lib/insn-eval.c +++ b/arch/x86/lib/insn-eval.c @@ -733,11 +733,11 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx) * * Returns: * - * A signed 8-bit value containing the default parameters on success. + * An int containing ORed-in default parameters on success. * * -EINVAL on error. */ -char insn_get_code_seg_params(struct pt_regs *regs) +int insn_get_code_seg_params(struct pt_regs *regs) { struct desc_struct *desc; short sel; -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86/insn-eval: negative return value? 2017-11-22 21:22 ` Borislav Petkov @ 2017-11-22 21:48 ` Kees Cook 2017-11-22 23:06 ` Borislav Petkov 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2017-11-22 21:48 UTC (permalink / raw) To: Borislav Petkov; +Cc: Ricardo Neri, LKML On Wed, Nov 22, 2017 at 1:22 PM, Borislav Petkov <bp@suse.de> wrote: > On Wed, Nov 22, 2017 at 12:45:17PM -0800, Kees Cook wrote: >> Hi, >> >> While doing some Clang test builds, this was reported: >> >> arch/x86/lib/insn-eval.c:780:10: warning: implicit conversion from >> 'int' to 'char' changes value from 132 to -124 [-Wconstant-conversion] >> return INSN_CODE_SEG_PARAMS(4, 8); >> ~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> ./arch/x86/include/asm/insn-eval.h:16:57: note: expanded from macro >> 'INSN_CODE_SEG_PARAMS' >> #define INSN_CODE_SEG_PARAMS(oper_sz, addr_sz) (oper_sz | (addr_sz << 4)) >> ~~~~~~~~^~~~~~~~~~~~~~~~ >> >> Is this really expected to wrap negative on IA-32e 64-bit mode case? > > Well, we were saving on structs and crammed two values into one but now > that I look at that function, it returns -EINVAL too, which is an int, > so its retval should simply be an int. IOW, I guess something like this: > > --- > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > index e1d3b4ce8a92..2b6ccf2c49f1 100644 > --- a/arch/x86/include/asm/insn-eval.h > +++ b/arch/x86/include/asm/insn-eval.h > @@ -18,6 +18,6 @@ > void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs); > unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx); > -char insn_get_code_seg_params(struct pt_regs *regs); > +int insn_get_code_seg_params(struct pt_regs *regs); > > #endif /* _ASM_X86_INSN_EVAL_H */ > diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c > index dabbac30acdf..f44ce0fb3583 100644 > --- a/arch/x86/kernel/umip.c > +++ b/arch/x86/kernel/umip.c > @@ -319,7 +319,7 @@ bool fixup_umip_exception(struct pt_regs *regs) > unsigned char buf[MAX_INSN_SIZE]; > void __user *uaddr; > struct insn insn; > - char seg_defs; > + int seg_defs; > > if (!regs) > return false; > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 35625d279458..9119d8e41f1f 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -733,11 +733,11 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx) > * > * Returns: > * > - * A signed 8-bit value containing the default parameters on success. > + * An int containing ORed-in default parameters on success. > * > * -EINVAL on error. > */ > -char insn_get_code_seg_params(struct pt_regs *regs) > +int insn_get_code_seg_params(struct pt_regs *regs) > { > struct desc_struct *desc; > short sel; > > -- > Regards/Gruss, > Boris. > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) > -- Cool, yeah, that looks like it would solve it. Thanks for taking a look! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86/insn-eval: negative return value? 2017-11-22 21:48 ` Kees Cook @ 2017-11-22 23:06 ` Borislav Petkov 2017-11-22 23:17 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Borislav Petkov @ 2017-11-22 23:06 UTC (permalink / raw) To: Kees Cook; +Cc: Ricardo Neri, LKML On Wed, Nov 22, 2017 at 01:48:55PM -0800, Kees Cook wrote: > Cool, yeah, that looks like it would solve it. Thanks for taking a look! Btw, how do you build the kernel with clang? I tried: make -j9 CC=clang HOSTCC=clang and clang here is: clang version 3.8.1-24 (tags/RELEASE_381/final) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin but I couldn't reproduce that warning. Thx. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86/insn-eval: negative return value? 2017-11-22 23:06 ` Borislav Petkov @ 2017-11-22 23:17 ` Kees Cook 2017-11-23 8:54 ` Borislav Petkov 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2017-11-22 23:17 UTC (permalink / raw) To: Borislav Petkov; +Cc: Ricardo Neri, LKML On Wed, Nov 22, 2017 at 3:06 PM, Borislav Petkov <bp@suse.de> wrote: > On Wed, Nov 22, 2017 at 01:48:55PM -0800, Kees Cook wrote: >> Cool, yeah, that looks like it would solve it. Thanks for taking a look! > > Btw, how do you build the kernel with clang? > > I tried: > > make -j9 CC=clang HOSTCC=clang > > and clang here is: > > clang version 3.8.1-24 (tags/RELEASE_381/final) clang-5.0 appears to be the minimum version needed (to produce something that actually boots and runs). $ clang-5.0 --version clang version 5.0.0-3 (tags/RELEASE_500/final) Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: x86/insn-eval: negative return value? 2017-11-22 23:17 ` Kees Cook @ 2017-11-23 8:54 ` Borislav Petkov 0 siblings, 0 replies; 6+ messages in thread From: Borislav Petkov @ 2017-11-23 8:54 UTC (permalink / raw) To: Kees Cook; +Cc: Ricardo Neri, LKML On Wed, Nov 22, 2017 at 03:17:09PM -0800, Kees Cook wrote: > clang-5.0 appears to be the minimum version needed (to produce > something that actually boots and runs). Yap, thanks, with that I was able to reproduce. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-23 8:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-22 20:45 x86/insn-eval: negative return value? Kees Cook 2017-11-22 21:22 ` Borislav Petkov 2017-11-22 21:48 ` Kees Cook 2017-11-22 23:06 ` Borislav Petkov 2017-11-22 23:17 ` Kees Cook 2017-11-23 8:54 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®