All of 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 related	[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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.