LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* 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;
as well as URLs for NNTP newsgroup(s).