Re: [PATCH] x86/kprobes: Fix 1 byte conditional jump target

[Nadav Amit <namit@vmware.com>]

> On Feb 6, 2023, at 8:42 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> !! External Email
> 
> On 2/4/23 13:08, Nadav Amit wrote:
>> --- a/arch/x86/kernel/kprobes/core.c
>> +++ b/arch/x86/kernel/kprobes/core.c
>> @@ -625,7 +625,7 @@ static int prepare_emulation(struct kprobe *p, struct insn *insn)
>>              /* 1 byte conditional jump */
>>              p->ainsn.emulate_op = kprobe_emulate_jcc;
>>              p->ainsn.jcc.type = opcode & 0xf;
>> -             p->ainsn.rel32 = *(char *)insn->immediate.bytes;
>> +             p->ainsn.rel32 = *(s8 *)&insn->immediate.value;
>>              break;
> 
> This new code is at least consistent with what the other code in that
> function does with 1-byte immediates.  But, I'm curious what the point
> is about going through the 's8' type.
> 
> What's wrong with:
> 
>        p->ainsn.rel32 = insn->immediate.value;
> 
> ?  Am I missing something subtle?

I am not sure why this is considered safe, insn->immediate.value has a
type of insn_value_t, which is signed int, so such casting seems wrong
to me. Do you imply that during decoding the sign-extension should have
been done correctly? Or am I missing something else?

Anyhow, after spending too much time on debugging kprobes failures,
I prefer to be more defensive, and not require the code to be “aware”
or rely on member types or the order of implicit casting in C.