Re: x86/insn-eval: negative return value?

[Borislav Petkov <bp@suse.de>]

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)
--