Messages in this thread | | | From | Nadav Amit <> | Subject | Re: [PATCH] x86/kprobes: Fix 1 byte conditional jump target | Date | Mon, 6 Feb 2023 19:05:02 +0000 |
| |
> 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.
| |