Message ID | 20230204210807.3930-1-namit@vmware.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp1513060wrn; Sat, 4 Feb 2023 13:28:49 -0800 (PST) X-Google-Smtp-Source: AK7set9kfaAHF7NbFAH0cEFR3l+QaaMuK5AP/UYpTH2GZgl83daBQG4XcPtkqj+1CHQ1SR3SBJj/ X-Received: by 2002:a05:6402:290c:b0:48e:94ec:b7ac with SMTP id ee12-20020a056402290c00b0048e94ecb7acmr12531752edb.7.1675546129333; Sat, 04 Feb 2023 13:28:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675546129; cv=none; d=google.com; s=arc-20160816; b=kHsMeaQZr/4lsrLyOzhi4CwQOZYvVJ7owGElPZcRQBosZN+RJDSgM/v9WyXIwD07gs DRJqKl7CENBQzy/d8zqZrbKlMAYtCx23QOqQ2aYkwFVzklg5JKpXpScXxgCM8t0IWh+A 4HkZrJKxupy7cigLqDP5l2GFc1UdErrsARxdqP13C35YthbcaUGPtyXWIk/0ssf0hJwL uzatvRHhEZOBhtFkaccnmxA0wlzRHBnxnf1TOpo1btV+Q/g6AYX48j4cA3no7Psv7nk3 HwLhW3sclgACchIFfrM8rwNEvOkbCmiZ22cLG9vLr+Ti0YvvZRfzHm/GICGdU9DnKuTW glXQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=JhJdmY9n68T9YoeltiaCGwu97Wb3Uy2dgfeoJGaZ/Mw=; b=vzuBYtByW31L/alud620A2Kw9IU/s0jnh4D241BqX869GpNuyVL8+/+vGEwog2JPef ZjcsgUPfQyumWIxKA500w9n7ahXZviWW32BGAiR4WrJWJ1haAzAmWo2MDLFfplIt0rsW j65u1B5q9cVRgYNWaMXPmpC8QjfDXk6SkhPUl36thrWXkiTYabL+RN7bEcFXoUg2mN8A nvRVJfLzljSYd+qEgiGNCJBK5xhrDoJTI/BJTgI0NtvhdqKYaSE0x9qeteiUw3ahthPm AjuFPwsmDWsjdmTgZIQGel5u9duj5dlJLP8qKJqEYXhJS9YulQASuY6pkYeHlECfmtNW u2ZA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="iDIxotW/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ey2-20020a0564022a0200b0049e420baaeesi7494953edb.470.2023.02.04.13.28.24; Sat, 04 Feb 2023 13:28:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b="iDIxotW/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232813AbjBDVIW (ORCPT <rfc822;il.mystafa@gmail.com> + 99 others); Sat, 4 Feb 2023 16:08:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34304 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231230AbjBDVIV (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 4 Feb 2023 16:08:21 -0500 Received: from mail-pj1-x102b.google.com (mail-pj1-x102b.google.com [IPv6:2607:f8b0:4864:20::102b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 457FE1206B for <linux-kernel@vger.kernel.org>; Sat, 4 Feb 2023 13:08:16 -0800 (PST) Received: by mail-pj1-x102b.google.com with SMTP id e10-20020a17090a630a00b0022bedd66e6dso11903649pjj.1 for <linux-kernel@vger.kernel.org>; Sat, 04 Feb 2023 13:08:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=JhJdmY9n68T9YoeltiaCGwu97Wb3Uy2dgfeoJGaZ/Mw=; b=iDIxotW/7OTSNGcbmxa8+muLWIqnTK8OFg6tZ55lJefql4omSvNQKuaCVZK1eUcqEw s0hEgJenBb3BiYKVKLUZXhdmCX2NwRxSWWEltHjY+3yFb/RGciuZ820QNcyRy68kuj5X tmhFgKcY1IhIdTV5YSKb4TM6fyUE1Wwlg5ClzVCsmGQZVRVAHDsxVYP6M78jzHRgqt2F 1V8h2BNLzFXvf6FohlGx3DARceX3lL6UIDy2t2aqpw3Q8crz/5S7Klcsr9kBaJCQaHI5 WxwVFBpS15MLvCghqYeN1p943QbdL2Ldke5HGC4ITOzjVssWj9STox8IwIi/LE8G34VM bJ+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=JhJdmY9n68T9YoeltiaCGwu97Wb3Uy2dgfeoJGaZ/Mw=; b=IH0KGUFMMeNu/LrIEwXGyaUxFYh+82k1NS8AqV+zHZb9z/Nl/oaEroFcSpWcEk/ahu mzxZFMvAhzdZJ2cuH02Bk/VlWC5PV5fx5g6jSvklDgjdm7n52DjDp6lKFEapj1W8e2CY YzuVi9FRzj3IAbS5EJ8rNMaCg4M1pdBIzk9pxl6yraD88OBh7tRoNDKSdhSOxXZR/gt1 /wH3nuG/mzYb96lje7vXz+/jKNbRaHMLfdukqVXOaVfoJMkOSx4bI7bLz/pYZBw40Elc le9Qx5XIwBy6gUQ7UMZQUXzJzNKCr4HC2IJ/mgPjuY2h3tQogK2SHvEqkI7yQPfctPer bVSA== X-Gm-Message-State: AO0yUKVuXJ+p3JJROS97Hm+rPbY4p8oxzAVkFor0drVgJh2rxzcxpqs+ 3NfaOaWznIKj2118L6YjfgM= X-Received: by 2002:a05:6a20:3c88:b0:bf:7b3a:fd59 with SMTP id b8-20020a056a203c8800b000bf7b3afd59mr15156235pzj.54.1675544895410; Sat, 04 Feb 2023 13:08:15 -0800 (PST) Received: from sc2-hs2-b1628.eng.vmware.com ([66.170.99.1]) by smtp.gmail.com with ESMTPSA id b10-20020a17090acc0a00b00219186abd7csm3781203pju.16.2023.02.04.13.08.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 04 Feb 2023 13:08:14 -0800 (PST) From: Nadav Amit <nadav.amit@gmail.com> X-Google-Original-From: Nadav Amit To: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, linux-kernel@vger.kernel.org, Nadav Amit <namit@vmware.com>, Masami Hiramatsu <mhiramat@kernel.org>, Peter Zijlstra <peterz@infradead.org> Subject: [PATCH] x86/kprobes: Fix 1 byte conditional jump target Date: Sat, 4 Feb 2023 21:08:07 +0000 Message-Id: <20230204210807.3930-1-namit@vmware.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1756937457975955186?= X-GMAIL-MSGID: =?utf-8?q?1756937457975955186?= |
Series |
x86/kprobes: Fix 1 byte conditional jump target
|
|
Commit Message
Nadav Amit
Feb. 4, 2023, 9:08 p.m. UTC
From: Nadav Amit <namit@vmware.com> Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") broke kprobes. Setting a probe-point on 1 byte conditional jump can cause the kernel to crash, as the branch target is not sign extended. Fix by using s8 instead of char and use immediate.value instead of immediate.bytes for consistency. Cc: Masami Hiramatsu (Google) <mhiramat@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Nadav Amit <namit@vmware.com> --- arch/x86/kernel/kprobes/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
> On Feb 4, 2023, at 11:08 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > > From: Nadav Amit <namit@vmware.com> > > Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") broke > kprobes. Setting a probe-point on 1 byte conditional jump can cause the > kernel to crash, as the branch target is not sign extended. > > Fix by using s8 instead of char and use immediate.value instead of > immediate.bytes for consistency. I guess I forgot to put a “Fixes” tag, since it is still not a real regression. (The bug was introduced in 6.2). Still, this fix should not fall between the cracks… Please let me know whether v2 is needed with a “fixes” tag.
On Sat, 4 Feb 2023 21:08:07 +0000 Nadav Amit <nadav.amit@gmail.com> wrote: > From: Nadav Amit <namit@vmware.com> > > Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") broke > kprobes. Setting a probe-point on 1 byte conditional jump can cause the > kernel to crash, as the branch target is not sign extended. Oops, indeed! > > Fix by using s8 instead of char and use immediate.value instead of > immediate.bytes for consistency. Looks good to me. Thanks for finding this bug! Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Fixes: 3bc753c06dd0 ("kbuild: treat char as always unsigned") Thank you! > > Cc: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arch/x86/kernel/kprobes/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > index b36f3c367cb2..6a56d56b3817 100644 > --- 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; > case 0x0f: > opcode = insn->opcode.bytes[1]; > -- > 2.34.1 >
On Sun, 5 Feb 2023 09:49:56 +0200 Nadav Amit <nadav.amit@gmail.com> wrote: > > > > On Feb 4, 2023, at 11:08 PM, Nadav Amit <nadav.amit@gmail.com> wrote: > > > > From: Nadav Amit <namit@vmware.com> > > > > Commit 3bc753c06dd0 ("kbuild: treat char as always unsigned") broke > > kprobes. Setting a probe-point on 1 byte conditional jump can cause the > > kernel to crash, as the branch target is not sign extended. > > > > Fix by using s8 instead of char and use immediate.value instead of > > immediate.bytes for consistency. > > I guess I forgot to put a “Fixes” tag, since it is still not a real > regression. (The bug was introduced in 6.2). > > Still, this fix should not fall between the cracks… Please let me know > whether v2 is needed with a “fixes” tag. No problem, I added Fixed tag with my Ack :) Thank you! >
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?
> 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.
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.
On Mon, 6 Feb 2023 14:38:16 -0800 Dave Hansen <dave.hansen@intel.com> 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? 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.
On Tue, 7 Feb 2023 09:54:24 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > On Mon, 6 Feb 2023 14:38:16 -0800 > Dave Hansen <dave.hansen@intel.com> 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@kernel.org>
On 2/7/23 07:21, Masami Hiramatsu (Google) wrote: >> 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, Let's focus on fixing the known bug first, please. Cleanups can come later.
> On Feb 7, 2023, at 5:33 PM, Dave Hansen <dave.hansen@intel.com> wrote: > > !! External Email > > On 2/7/23 07:21, Masami Hiramatsu (Google) wrote: >>> 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, > > Let's focus on fixing the known bug first, please. Cleanups can come later. Thank you Dave. That was my take too following your email. I certainly did not pay attention to the fact that sign extension has already been done in insn_get_immediate() before Masami pointed it out. So, the comment in insn_get_immediate() should also be updated to note that the immediate is sign-*extended* in *all* cases (instead of sign-expanded in most cases. :) ) As you said, I guess the change you and Masami proposed can be done on top of this patch, which is (and was) only intended to fix the bug, and should therefore go into 6.2.
On 2/7/23 22:34, Nadav Amit wrote: > As you said, I guess the change you and Masami proposed can be done on top of > this patch, which is (and was) only intended to fix the bug, and should > therefore go into 6.2. Nadav, To fix this issue, you proposed: p->ainsn.rel32 = *(s8 *)&insn->immediate.value; But, this is, um, rather obfuscated and potentially less correct compared to: p->ainsn.rel32 = insn->immediate.value; I'd appreciate it if you could update your patch to do this simpler thing and resend, unless there is a strong reason to do what you originally proposed.
> On Feb 8, 2023, at 8:56 AM, Dave Hansen <dave.hansen@intel.com> wrote: > > !! External Email > > On 2/7/23 22:34, Nadav Amit wrote: >> As you said, I guess the change you and Masami proposed can be done on top of >> this patch, which is (and was) only intended to fix the bug, and should >> therefore go into 6.2. > > Nadav, > > To fix this issue, you proposed: > > p->ainsn.rel32 = *(s8 *)&insn->immediate.value; > > But, this is, um, rather obfuscated and potentially less correct > compared to: > > p->ainsn.rel32 = insn->immediate.value; > > I'd appreciate it if you could update your patch to do this simpler > thing and resend, unless there is a strong reason to do what you > originally proposed. I thought it makes sense to make this change for all the other cases in one pass, but whatever makes you happy. I will send v2 soon. Thanks again, Nadav
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index b36f3c367cb2..6a56d56b3817 100644 --- 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; case 0x0f: opcode = insn->opcode.bytes[1];