Message ID | 20230102160748.1307289-1-bjorn@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp4222812wrt; Mon, 2 Jan 2023 08:13:17 -0800 (PST) X-Google-Smtp-Source: AMrXdXvYCj566NkAv724yKeujNz5dQERa8skXOIDHOz8uVzgfWO5+Gwert4UAgvuYWUuogJIVExC X-Received: by 2002:a05:6402:4013:b0:47e:869:9115 with SMTP id d19-20020a056402401300b0047e08699115mr8864636eda.3.1672675997246; Mon, 02 Jan 2023 08:13:17 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1672675997; cv=none; d=google.com; s=arc-20160816; b=qiAkSWt27NciL1ljaZtNQ4uVNDpRw+NrE1eiEXIfowNwLV770ryBiCstolNhfhyG/s 5AKweuIPux64YgTmh5FIMzIcPxws2+dVEhPOiCdUCi/CoU1xs2XQ5v7ptX6YyCGh2F9Z z7MANj5zF9i/rereIHLkZxpwdjSvHUYFE1AqSDOBa7yhP7WoZVuRiBOMG9hmGOwiu8lf hsC+tECm4COcfk6l/Qtn/1ONgjPGPyl6QGKkHdhrk7NFhuy98NuivYoCcNwzIIQ26bDQ i3Zg6KthxiaYL1ngs0P0JV2gNpyJF1QdR5UMiNpdAXQdepEoIP1Ol85U1kTtLhoWwhmS ohsQ== 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=z+iNiYQzY6dLXCTXiG6GIDgbPK977fsSEdtTxOIhlZ8=; b=GqzcAjmxBkXkOSRcvvH5/f9psE/026fIXlJCMZ96Xzwjb6beI//zMw3bsSmsDfB0d8 dHntlpgGMHqRtqKvt2hHgHDa8A83TBiwn34jfiK1nzQq6M9ZYwmNoiSNTF8oLYLTGyQY kKMIRKN7u+Cch//yH5LLm1FshSB+oDlpGSbv7GR3G16fsN9B5zCbO1aXdr5AGmn5D1Qg Em4/Iq+wYzta/wy+f6JzeoZse5vKq0GFZTb7RNwfogIiZd6xOsnkXTYk/zt1ooqb5XAN f+VASQDkI5mvFgOn4iwe61e3BnPH0jIaHDjvPWSlN9dza/vEEGK9u+Ak9euxxIB9a5hm V+gA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=gDRaFyOH; 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=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b11-20020a0564021f0b00b0048ea97eec14si821912edb.609.2023.01.02.08.12.53; Mon, 02 Jan 2023 08:13:17 -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=@kernel.org header.s=k20201202 header.b=gDRaFyOH; 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=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236163AbjABQIL (ORCPT <rfc822;yugsuo@gmail.com> + 99 others); Mon, 2 Jan 2023 11:08:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54062 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236603AbjABQIC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 2 Jan 2023 11:08:02 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 67D8EB7C2; Mon, 2 Jan 2023 08:07:58 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E7E7C60C2A; Mon, 2 Jan 2023 16:07:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ABB6CC433EF; Mon, 2 Jan 2023 16:07:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1672675677; bh=AO3Ixc0TR11NeS3MBCzLhmAhDwnDKeJ/27og6j1PuYM=; h=From:To:Cc:Subject:Date:From; b=gDRaFyOHdrdRw8JvQAgaQ3UkwUJGEFtMQyT3fDX9inWxwCXnFxV4Oj35qM87PjeP7 4IiGHpaOVpM9BRytyMeQeCmn8ybG3xGtVKC2lVBvNiOWqL8Hk3Z/Q8+ESQFcSUO0UG dig9tg9miI8hw6mNtsgTDqJMvAA2i2EL+2eZa5UosoQ1wrSKeoMj0NSXFEUFlGSfL4 MS+Cm3dUcgcOg3FrRf4j4LdNbczf9fHEhxKZ9taF9e0yeVw/D0GtkwNnI+F9KM9Xnt Md4VrHmsWfgc4yVmWOVfoKqTwV5bBXp16YEyc1yZL3Ry7uMtLi8Xtx9LIBw/4WrzyC QMZYEJmpyH3Hg== From: =?utf-8?b?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org> To: Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, linux-riscv@lists.infradead.org, Guo Ren <guoren@kernel.org> Cc: =?utf-8?b?QmrDtnJuIFTDtnBlbA==?= <bjorn@rivosinc.com>, linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org Subject: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding Date: Mon, 2 Jan 2023 17:07:48 +0100 Message-Id: <20230102160748.1307289-1-bjorn@kernel.org> X-Mailer: git-send-email 2.37.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS 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?1753927906910027389?= X-GMAIL-MSGID: =?utf-8?q?1753927906910027389?= |
Series |
riscv, kprobes: Stricter c.jr/c.jalr decoding
|
|
Commit Message
Björn Töpel
Jan. 2, 2023, 4:07 p.m. UTC
From: Björn Töpel <bjorn@rivosinc.com> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add is encoded the following way (each instruction is 16b): ---+-+-----------+-----------+-- 100 0 rs1[4:0]!=0 00000 10 : c.jr 100 1 rs1[4:0]!=0 00000 10 : c.jalr 100 0 rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv 100 1 rd[4:0]!=0 rs2[4:0]!=0 10 : c.add The following logic is used to decode c.jr and c.jalr: insn & 0xf007 == 0x8002 => instruction is an c.jr insn & 0xf007 == 0x9002 => instruction is an c.jalr When 0xf007 is used to mask the instruction, c.mv can be incorrectly decoded as c.jr, and c.add as c.jalr. Correct the decoding by changing the mask from 0xf007 to 0xf07f. Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported") Signed-off-by: Björn Töpel <bjorn@rivosinc.com> --- arch/riscv/kernel/probes/simulate-insn.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
Comments
Hey Bjorn, On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add > is encoded the following way (each instruction is 16b): > > ---+-+-----------+-----------+-- > 100 0 rs1[4:0]!=0 00000 10 : c.jr > 100 1 rs1[4:0]!=0 00000 10 : c.jalr > 100 0 rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv > 100 1 rd[4:0]!=0 rs2[4:0]!=0 10 : c.add > > The following logic is used to decode c.jr and c.jalr: > > insn & 0xf007 == 0x8002 => instruction is an c.jr > insn & 0xf007 == 0x9002 => instruction is an c.jalr > > When 0xf007 is used to mask the instruction, c.mv can be incorrectly > decoded as c.jr, and c.add as c.jalr. > > Correct the decoding by changing the mask from 0xf007 to 0xf07f. > > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > arch/riscv/kernel/probes/simulate-insn.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h > index cb6ff7dccb92..de8474146a9b 100644 > --- a/arch/riscv/kernel/probes/simulate-insn.h > +++ b/arch/riscv/kernel/probes/simulate-insn.h > @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f); > } while (0) > > __RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001); > -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002); Hmm, I wonder where the mask originally came from! I had a look at the compressed spec, of which the version google gave to me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC, Quadrant 2) seems to list them all together. Tedious it may be, but future instruction decoding bits probably need more scrutiny as Drew found another clearly wrong bit a few weeks ago [2]. Anyways, I checked against the doc and the new versions look good to me. How'd you spot this, and did you check the other masks? Reviewed-by: Conor Dooley <conor.dooley@microchip.com> [1] - https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf [2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/ > +__RISCV_INSN_FUNCS(c_jr, 0xf07f, 0x8002); > __RISCV_INSN_FUNCS(c_jal, 0xe003, 0x2001); > -__RISCV_INSN_FUNCS(c_jalr, 0xf007, 0x9002); > +__RISCV_INSN_FUNCS(c_jalr, 0xf07f, 0x9002); > __RISCV_INSN_FUNCS(c_beqz, 0xe003, 0xc001); > __RISCV_INSN_FUNCS(c_bnez, 0xe003, 0xe001); > __RISCV_INSN_FUNCS(c_ebreak, 0xffff, 0x9002); Worth noting that this code is gone in riscv/for-next thanks to Heiko's de-duplication: https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/ > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2 > -- > 2.37.2 >
Conor Dooley <conor@kernel.org> writes: > Hey Bjorn, > > On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote: >> From: Björn Töpel <bjorn@rivosinc.com> >> >> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add >> is encoded the following way (each instruction is 16b): >> >> ---+-+-----------+-----------+-- >> 100 0 rs1[4:0]!=0 00000 10 : c.jr >> 100 1 rs1[4:0]!=0 00000 10 : c.jalr >> 100 0 rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv >> 100 1 rd[4:0]!=0 rs2[4:0]!=0 10 : c.add >> >> The following logic is used to decode c.jr and c.jalr: >> >> insn & 0xf007 == 0x8002 => instruction is an c.jr >> insn & 0xf007 == 0x9002 => instruction is an c.jalr >> >> When 0xf007 is used to mask the instruction, c.mv can be incorrectly >> decoded as c.jr, and c.add as c.jalr. >> >> Correct the decoding by changing the mask from 0xf007 to 0xf07f. >> >> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported") >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >> --- >> arch/riscv/kernel/probes/simulate-insn.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h >> index cb6ff7dccb92..de8474146a9b 100644 >> --- a/arch/riscv/kernel/probes/simulate-insn.h >> +++ b/arch/riscv/kernel/probes/simulate-insn.h >> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f); >> } while (0) >> >> __RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001); >> -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002); > > Hmm, I wonder where the mask originally came from! I think it's just a simple bug -- missing that "rs2" must be zero. > I had a look at the compressed spec, of which the version google gave to > me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC, > Quadrant 2) seems to list them all together. > Tedious it may be, but future instruction decoding bits probably need > more scrutiny as Drew found another clearly wrong bit a few weeks ago > [2]. > > Anyways, I checked against the doc and the new versions look good to > me. How'd you spot this, and did you check the other masks? I got hit by it when testing the optprobe series (c.mv was rejected as c.jr). Skimmed the other masks quickly, but will take another look. > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > [1] - > https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf C-ext is part of the unpriv spec: https://github.com/riscv/riscv-isa-manual/releases > [2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/ > >> +__RISCV_INSN_FUNCS(c_jr, 0xf07f, 0x8002); >> __RISCV_INSN_FUNCS(c_jal, 0xe003, 0x2001); >> -__RISCV_INSN_FUNCS(c_jalr, 0xf007, 0x9002); >> +__RISCV_INSN_FUNCS(c_jalr, 0xf07f, 0x9002); >> __RISCV_INSN_FUNCS(c_beqz, 0xe003, 0xc001); >> __RISCV_INSN_FUNCS(c_bnez, 0xe003, 0xe001); >> __RISCV_INSN_FUNCS(c_ebreak, 0xffff, 0x9002); > > Worth noting that this code is gone in riscv/for-next thanks to Heiko's > de-duplication: > https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/ Yay! Björn
Thx for catching it. On Tue, Jan 3, 2023 at 12:07 AM Björn Töpel <bjorn@kernel.org> wrote: > > From: Björn Töpel <bjorn@rivosinc.com> > > In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add > is encoded the following way (each instruction is 16b): > > ---+-+-----------+-----------+-- > 100 0 rs1[4:0]!=0 00000 10 : c.jr > 100 1 rs1[4:0]!=0 00000 10 : c.jalr > 100 0 rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv > 100 1 rd[4:0]!=0 rs2[4:0]!=0 10 : c.add Yes, I forgot the c.mv & c.add effect. Reviewed-by: Guo Ren <guoren@kernel.org> > > The following logic is used to decode c.jr and c.jalr: > > insn & 0xf007 == 0x8002 => instruction is an c.jr > insn & 0xf007 == 0x9002 => instruction is an c.jalr > > When 0xf007 is used to mask the instruction, c.mv can be incorrectly > decoded as c.jr, and c.add as c.jalr. > > Correct the decoding by changing the mask from 0xf007 to 0xf07f. > > Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > arch/riscv/kernel/probes/simulate-insn.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h > index cb6ff7dccb92..de8474146a9b 100644 > --- a/arch/riscv/kernel/probes/simulate-insn.h > +++ b/arch/riscv/kernel/probes/simulate-insn.h > @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f); > } while (0) > > __RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001); > -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002); > +__RISCV_INSN_FUNCS(c_jr, 0xf07f, 0x8002); > __RISCV_INSN_FUNCS(c_jal, 0xe003, 0x2001); > -__RISCV_INSN_FUNCS(c_jalr, 0xf007, 0x9002); > +__RISCV_INSN_FUNCS(c_jalr, 0xf07f, 0x9002); > __RISCV_INSN_FUNCS(c_beqz, 0xe003, 0xc001); > __RISCV_INSN_FUNCS(c_bnez, 0xe003, 0xe001); > __RISCV_INSN_FUNCS(c_ebreak, 0xffff, 0x9002); > > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2 > -- > 2.37.2 >
On Tue, Jan 03, 2023 at 07:44:49AM +0100, Björn Töpel wrote: > Conor Dooley <conor@kernel.org> writes: > > On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote: > >> From: Björn Töpel <bjorn@rivosinc.com> > >> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h > >> index cb6ff7dccb92..de8474146a9b 100644 > >> --- a/arch/riscv/kernel/probes/simulate-insn.h > >> +++ b/arch/riscv/kernel/probes/simulate-insn.h > >> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f); > >> } while (0) > >> > >> __RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001); > >> -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002); > > > > Hmm, I wonder where the mask originally came from! > > I think it's just a simple bug -- missing that "rs2" must be zero. > > > I had a look at the compressed spec, of which the version google gave to > > me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC, > > Quadrant 2) seems to list them all together. > > [1] - > > https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf > > C-ext is part of the unpriv spec: > https://github.com/riscv/riscv-isa-manual/releases Yah, I was trying to see if there was some period piece which was misleading that would have explained the mask. I looked again & the v1.7 spec doesn't have that table, but also has no reason to suggest the current mask either. Guess it was just a mistake :)
Hello: This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <palmer@rivosinc.com>: On Mon, 2 Jan 2023 17:07:48 +0100 you wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add > is encoded the following way (each instruction is 16b): > > ---+-+-----------+-----------+-- > 100 0 rs1[4:0]!=0 00000 10 : c.jr > 100 1 rs1[4:0]!=0 00000 10 : c.jalr > 100 0 rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv > 100 1 rd[4:0]!=0 rs2[4:0]!=0 10 : c.add > > [...] Here is the summary with links: - riscv, kprobes: Stricter c.jr/c.jalr decoding https://git.kernel.org/riscv/c/b2d473a6019e You are awesome, thank you!
diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h index cb6ff7dccb92..de8474146a9b 100644 --- a/arch/riscv/kernel/probes/simulate-insn.h +++ b/arch/riscv/kernel/probes/simulate-insn.h @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence, 0x7f, 0x0f); } while (0) __RISCV_INSN_FUNCS(c_j, 0xe003, 0xa001); -__RISCV_INSN_FUNCS(c_jr, 0xf007, 0x8002); +__RISCV_INSN_FUNCS(c_jr, 0xf07f, 0x8002); __RISCV_INSN_FUNCS(c_jal, 0xe003, 0x2001); -__RISCV_INSN_FUNCS(c_jalr, 0xf007, 0x9002); +__RISCV_INSN_FUNCS(c_jalr, 0xf07f, 0x9002); __RISCV_INSN_FUNCS(c_beqz, 0xe003, 0xc001); __RISCV_INSN_FUNCS(c_bnez, 0xe003, 0xe001); __RISCV_INSN_FUNCS(c_ebreak, 0xffff, 0x9002);