Message-ID: <dc964552-dca7-dd83-52a2-283be7f51555@intel.com>
Date:   Mon, 6 Feb 2023 14:38:16 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] x86/kprobes: Fix 1 byte conditional jump target
Content-Language: en-US
To:     Nadav Amit <namit@vmware.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        X86 ML <x86@kernel.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
References: <20230204210807.3930-1-namit@vmware.com>
 <a75c134d-8278-b17c-e05e-409b70ad15b0@intel.com>
 <C4863EDA-106B-4AF9-8D39-D603EEE4BEDC@vmware.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <C4863EDA-106B-4AF9-8D39-D603EEE4BEDC@vmware.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

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