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

[Kees Cook]

On Wed, Nov 22, 2017 at 1:22 PM, Borislav Petkov <bp@xxxxxxx> 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