[PATCH 0/3] riscv: kgdb: Fix break size, insn patch and masking

[PATCH 0/3] riscv: kgdb: Fix break size, insn patch and masking

[Clément Léger]

This small series fixes a few problems that were spotted with kgdb.

First patch is the use of patch_insn_write() to set breakpoints instead
of copy_to_kernel_nofault(). This fixes the use of kgdb with
CONFIG_STRICT_KERNEL_RWX which maps text as RX.

Second patch removes the hardcoded epc += 4 with BREAK_INSTR_SIZE so
that it works with CONFIG_RISCV_ISA_C.

Last one is more of a correctness fix with respect to jalr instructions.
ISA specifies that the target address is computed by masking the LSB to
0.

Clément Léger (3):
  riscv: kgdb: use patch_insn_write() to set breakpoints
  riscv: kgdb: use BREAK_INSTR_SIZE instead of hardcoded value
  riscv: kgdb: clear bit 0 of single-step jump targets

 arch/riscv/kernel/kgdb.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

-- 
Clément Léger

[PATCH 0/3] riscv: kgdb: Fix break size, insn patch and masking

[Clément Léger]

This small series fixes a few problems that were spotted with kgdb.

First patch is the use of patch_insn_write() to set breakpoints instead
of copy_to_kernel_nofault(). This fixes the use of kgdb with
CONFIG_STRICT_KERNEL_RWX which maps text as RX.

Second patch removes the hardcoded epc += 4 with BREAK_INSTR_SIZE so
that it works with CONFIG_RISCV_ISA_C.

Last one is more of a correctness fix with respect to jalr instructions.
ISA specifies that the target address is computed by masking the LSB to
0.

Clément Léger (3):
  riscv: kgdb: use patch_insn_write() to set breakpoints
  riscv: kgdb: use BREAK_INSTR_SIZE instead of hardcoded value
  riscv: kgdb: clear bit 0 of single-step jump targets

 arch/riscv/kernel/kgdb.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

-- 
Clément Léger

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/3] riscv: kgdb: use patch_insn_write() to set breakpoints

[Clément Léger]

When using CONFIG_STRICT_KERNEL_RWX, the text section is not writable
and copy_to_kernel_nofault() returns -EFAULT. Fix this by using
patch_insn_write() were applicable and implement
kgdb_arch_set_breakpoint()/kgdb_arch_remove_breakpoint().

patch_insn_write() correctly remaps the text as writable using a fixmap
alias.

Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 0bf629204c76..4ad817698af8 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -12,6 +12,7 @@
 #include <asm/cacheflush.h>
 #include <asm/gdb_xml.h>
 #include <asm/insn.h>
+#include <asm/text-patching.h>
 
 enum {
 	NOT_KGDB_BREAK = 0,
@@ -134,9 +135,9 @@ static int do_single_step(struct pt_regs *regs)
 	stepped_address = addr;
 
 	/* Replace the op code with the break instruction */
-	error = copy_to_kernel_nofault((void *)stepped_address,
-				   arch_kgdb_ops.gdb_bpt_instr,
-				   BREAK_INSTR_SIZE);
+	error = patch_insn_write((void *)stepped_address,
+				 arch_kgdb_ops.gdb_bpt_instr,
+				 BREAK_INSTR_SIZE);
 	/* Flush and return */
 	if (!error) {
 		flush_icache_range(addr, addr + BREAK_INSTR_SIZE);
@@ -154,8 +155,8 @@ static int do_single_step(struct pt_regs *regs)
 static void undo_single_step(struct pt_regs *regs)
 {
 	if (stepped_opcode != 0) {
-		copy_to_kernel_nofault((void *)stepped_address,
-				   (void *)&stepped_opcode, BREAK_INSTR_SIZE);
+		patch_insn_write((void *)stepped_address,
+				 (void *)&stepped_opcode, BREAK_INSTR_SIZE);
 		flush_icache_range(stepped_address,
 				   stepped_address + BREAK_INSTR_SIZE);
 	}
@@ -261,6 +262,29 @@ noinline void arch_kgdb_breakpoint(void)
 	    "kgdb_compiled_break: ebreak\n");
 }
 
+/*
+ * Kernel text may be read-only (CONFIG_STRICT_KERNEL_RWX), so software
+ * breakpoints must be planted through the writable patch alias instead of
+ * writing the text address directly.
+ */
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+	int err;
+
+	err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
+				       BREAK_INSTR_SIZE);
+	if (err)
+		return err;
+	return patch_insn_write((void *)bpt->bpt_addr,
+				arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+	return patch_insn_write((void *)bpt->bpt_addr, bpt->saved_instr,
+				BREAK_INSTR_SIZE);
+}
+
 void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer,
 				char *remcom_out_buffer)
 {
-- 
2.55.0

[PATCH 1/3] riscv: kgdb: use patch_insn_write() to set breakpoints

[Clément Léger]

When using CONFIG_STRICT_KERNEL_RWX, the text section is not writable
and copy_to_kernel_nofault() returns -EFAULT. Fix this by using
patch_insn_write() were applicable and implement
kgdb_arch_set_breakpoint()/kgdb_arch_remove_breakpoint().

patch_insn_write() correctly remaps the text as writable using a fixmap
alias.

Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 0bf629204c76..4ad817698af8 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -12,6 +12,7 @@
 #include <asm/cacheflush.h>
 #include <asm/gdb_xml.h>
 #include <asm/insn.h>
+#include <asm/text-patching.h>
 
 enum {
 	NOT_KGDB_BREAK = 0,
@@ -134,9 +135,9 @@ static int do_single_step(struct pt_regs *regs)
 	stepped_address = addr;
 
 	/* Replace the op code with the break instruction */
-	error = copy_to_kernel_nofault((void *)stepped_address,
-				   arch_kgdb_ops.gdb_bpt_instr,
-				   BREAK_INSTR_SIZE);
+	error = patch_insn_write((void *)stepped_address,
+				 arch_kgdb_ops.gdb_bpt_instr,
+				 BREAK_INSTR_SIZE);
 	/* Flush and return */
 	if (!error) {
 		flush_icache_range(addr, addr + BREAK_INSTR_SIZE);
@@ -154,8 +155,8 @@ static int do_single_step(struct pt_regs *regs)
 static void undo_single_step(struct pt_regs *regs)
 {
 	if (stepped_opcode != 0) {
-		copy_to_kernel_nofault((void *)stepped_address,
-				   (void *)&stepped_opcode, BREAK_INSTR_SIZE);
+		patch_insn_write((void *)stepped_address,
+				 (void *)&stepped_opcode, BREAK_INSTR_SIZE);
 		flush_icache_range(stepped_address,
 				   stepped_address + BREAK_INSTR_SIZE);
 	}
@@ -261,6 +262,29 @@ noinline void arch_kgdb_breakpoint(void)
 	    "kgdb_compiled_break: ebreak\n");
 }
 
+/*
+ * Kernel text may be read-only (CONFIG_STRICT_KERNEL_RWX), so software
+ * breakpoints must be planted through the writable patch alias instead of
+ * writing the text address directly.
+ */
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+	int err;
+
+	err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
+				       BREAK_INSTR_SIZE);
+	if (err)
+		return err;
+	return patch_insn_write((void *)bpt->bpt_addr,
+				arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+	return patch_insn_write((void *)bpt->bpt_addr, bpt->saved_instr,
+				BREAK_INSTR_SIZE);
+}
+
 void kgdb_arch_handle_qxfer_pkt(char *remcom_in_buffer,
 				char *remcom_out_buffer)
 {
-- 
2.55.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/3] riscv: kgdb: use BREAK_INSTR_SIZE instead of hardcoded value

[Clément Léger]

Before commit 550c2aa787d1 ("riscv: KGDB: Remove ".option norvc/.option
rvc" for kgdb_compiled_break"), .norvc was enforced and that resulted
in a non compressed ebreak instruction so the +4 was legit. The
previously mentionned commit removed that option, leading to ebreak
being emitted as c.ebreak when compiled with CONFIG_RISCV_ISA_C=y.

Use BREAK_INSTR_SIZE so epc is incremented by whatever size the break
instruction is.

Fixes: 550c2aa787d1 ("riscv: KGDB: Remove ".option norvc/.option rvc" for kgdb_compiled_break")
Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 4ad817698af8..121dae87dc68 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -365,7 +365,7 @@ static int kgdb_riscv_notify(struct notifier_block *self, unsigned long cmd,
 		return NOTIFY_DONE;
 
 	if (type == KGDB_COMPILED_BREAK)
-		regs->epc += 4;
+		regs->epc += BREAK_INSTR_SIZE;
 
 	local_irq_restore(flags);
 
-- 
2.55.0

[PATCH 2/3] riscv: kgdb: use BREAK_INSTR_SIZE instead of hardcoded value

[Clément Léger]

Before commit 550c2aa787d1 ("riscv: KGDB: Remove ".option norvc/.option
rvc" for kgdb_compiled_break"), .norvc was enforced and that resulted
in a non compressed ebreak instruction so the +4 was legit. The
previously mentionned commit removed that option, leading to ebreak
being emitted as c.ebreak when compiled with CONFIG_RISCV_ISA_C=y.

Use BREAK_INSTR_SIZE so epc is incremented by whatever size the break
instruction is.

Fixes: 550c2aa787d1 ("riscv: KGDB: Remove ".option norvc/.option rvc" for kgdb_compiled_break")
Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 4ad817698af8..121dae87dc68 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -365,7 +365,7 @@ static int kgdb_riscv_notify(struct notifier_block *self, unsigned long cmd,
 		return NOTIFY_DONE;
 
 	if (type == KGDB_COMPILED_BREAK)
-		regs->epc += 4;
+		regs->epc += BREAK_INSTR_SIZE;
 
 	local_irq_restore(flags);
 
-- 
2.55.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/3] riscv: kgdb: clear bit 0 of single-step jump targets

[Clément Léger]

Per the RISC-V ISA, JALR (and C.JR/C.JALR) clear the lowest bit of
the calculated target address, so that auxiliary information can be
stored in the low bit of function pointers. kgdb's software single
step must compute the same target the hardware will actually take.

This mirrors what the kprobe instruction simulator already does in
simulate_jalr().

Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 121dae87dc68..647560194d8c 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -48,7 +48,7 @@ static int get_step_address(struct pt_regs *regs, unsigned long *next_addr)
 		if (riscv_insn_is_c_jalr(op_code) ||
 		    riscv_insn_is_c_jr(op_code)) {
 			rs1_num = decode_register_index(op_code, RVC_C2_RS1_OPOFF);
-			*next_addr = regs_ptr[rs1_num];
+			*next_addr = regs_ptr[rs1_num] & ~1UL;
 		} else if (riscv_insn_is_c_j(op_code) ||
 			   riscv_insn_is_c_jal(op_code)) {
 			*next_addr = RVC_EXTRACT_JTYPE_IMM(op_code) + pc;
@@ -109,6 +109,7 @@ static int get_step_address(struct pt_regs *regs, unsigned long *next_addr)
 			if (rs1_num)
 				*next_addr = ((unsigned long *)regs)[rs1_num];
 			*next_addr += RV_EXTRACT_ITYPE_IMM(op_code);
+			*next_addr &= ~1UL;
 		} else if (riscv_insn_is_sret(op_code)) {
 			*next_addr = pc;
 		} else {
-- 
2.55.0

[PATCH 3/3] riscv: kgdb: clear bit 0 of single-step jump targets

[Clément Léger]

Per the RISC-V ISA, JALR (and C.JR/C.JALR) clear the lowest bit of
the calculated target address, so that auxiliary information can be
stored in the low bit of function pointers. kgdb's software single
step must compute the same target the hardware will actually take.

This mirrors what the kprobe instruction simulator already does in
simulate_jalr().

Signed-off-by: Clément Léger <clement.leger@linux.dev>
---
 arch/riscv/kernel/kgdb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/kgdb.c b/arch/riscv/kernel/kgdb.c
index 121dae87dc68..647560194d8c 100644
--- a/arch/riscv/kernel/kgdb.c
+++ b/arch/riscv/kernel/kgdb.c
@@ -48,7 +48,7 @@ static int get_step_address(struct pt_regs *regs, unsigned long *next_addr)
 		if (riscv_insn_is_c_jalr(op_code) ||
 		    riscv_insn_is_c_jr(op_code)) {
 			rs1_num = decode_register_index(op_code, RVC_C2_RS1_OPOFF);
-			*next_addr = regs_ptr[rs1_num];
+			*next_addr = regs_ptr[rs1_num] & ~1UL;
 		} else if (riscv_insn_is_c_j(op_code) ||
 			   riscv_insn_is_c_jal(op_code)) {
 			*next_addr = RVC_EXTRACT_JTYPE_IMM(op_code) + pc;
@@ -109,6 +109,7 @@ static int get_step_address(struct pt_regs *regs, unsigned long *next_addr)
 			if (rs1_num)
 				*next_addr = ((unsigned long *)regs)[rs1_num];
 			*next_addr += RV_EXTRACT_ITYPE_IMM(op_code);
+			*next_addr &= ~1UL;
 		} else if (riscv_insn_is_sret(op_code)) {
 			*next_addr = pc;
 		} else {
-- 
2.55.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv