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

[Google]

On Tue, 7 Feb 2023 09:54:24 +0900
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> wrote:

> On Mon, 6 Feb 2023 14:38:16 -0800
> Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> 
> > On 2/6/23 11:05, Nadav Amit wrote:
> > >> 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?
> > 
> > OK, so we've got an assignment which on the left hand side is
> > p->ainsn.rel32 which is a 32-bit signed integer:
> > 
> > struct arch_specific_insn {
> > 	...
> >         s32 rel32;      /* relative offset must be s32, s16, or s8 */
> > 
> > The right hand side is insn->immediate.value.  Its real type is a couple
> > of layers deep, but it boils down to a 'signed int', also 32-bit:
> > 
> > Struct #1:
> > struct insn {
> > 	...
> >         union {
> >                 struct insn_field immediate;
> > 		...
> >         };
> > 
> > Struct #2
> > struct insn_field {
> >         union {
> >                 insn_value_t value;
> >                 insn_byte_t bytes[4];
> >         };
> > 	...
> > 
> > And a typedef:
> > typedef signed int insn_value_t;
> > 
> > So, the proposed code above is effectively this:
> > 
> > 	s32 foo;
> > 	signed int bar;
> > 
> > 	foo = *(s8 *)&bar;
> > 
> > That works just fine as long as the value being represented fits in a
> > single byte.  But, it *certainly* wouldn't work for:
> > 
> > 	s32 foo;
> > 	signed int bar = 128;
> > 
> > 	foo = *(s8 *)&bar;
> > 
> > In this specific case, I think the conditional jump offsets are all from
> > the (entire) second byte of the instruction, so this is _somewhat_ academic.
> 
> NOTE: Since we have checked the opcode is Jcc (0x70 to 0x7f) we ensured that
> the immediate value is 1 byte (rel8 = -128 to +127).
> 
>         case 0x70 ... 0x7f:
>                 /* 1 byte conditional jump */
>                 p->ainsn.emulate_op = kprobe_emulate_jcc;
>                 p->ainsn.jcc.type = opcode & 0xf;
>                 p->ainsn.rel32 = *(s8 *)&insn->immediate.value;
>                 break;
> 
> But I think your have a point. I missed that Nadav is using immediate.value
> instead of immediate.bytes[0]. And from the instruction decoder code, it is
> better to use immediate.value without casting.
> 
> In arch/x86/lib/insn.c:
> 
> int insn_get_immediate(struct insn *insn)
> {
> ...
>         switch (inat_immediate_size(insn->attr)) {
>         case INAT_IMM_BYTE:
>                 insn_field_set(&insn->immediate, get_next(signed char, insn), 1);
>                 break;
> 
> And 
> 
> In arch/x86/include/asm/insn.h:
> 
> static inline void insn_field_set(struct insn_field *p, insn_value_t v,
>                                   unsigned char n)
> {
>         p->value = v;
>         p->nbytes = n;
> }
> 
> Thus the immediate.value should be set correctly. (means we don't have to
> pick up the 1st byte from the value)
> 
> Nadav, can you update your patch to assign immediate.value directly?

BTW, there are many similar casts around there. I'll fix those too.
If we need to be more conservative, 

 p->ainsn.rel32 = (s8)insn->immediate.value;

should work, right?
Or, maybe we can add WARN_ON_ONCE() as

 WARN_ON_ONCE(insn.immediate.nbytes != 1)

Thank you, 

> 
> Thank you,
> 
> > 
> > > 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.
> > 
> > Well, the code in the fix requires some awareness of the range of the
> > data type.  The simpler direct assignment:
> > 
> > 	p->ainsn.rel32 = insn->immediate.value;
> > 
> > doesn't require much and works for a wider range of values -- *ALL*
> > 32-bit signed integer values on x86.
> > 
> > I figured I must be missing something.  It would not be the first time
> > that C's type system rules tripped me up.  Why this:
> > 
> > 	foo = *(s8 *)&bar;
> > 
> > Instead of this:
> > 
> > 	foo = bar;
> > 
> > ?  I'm having a hard time of seeing what the advantage is of the 's8'
> > version.
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>