Sun May 23 23:22:55 2021 UTC ()
Improve ddb disassembly for mips (and riscv, cribbed from mips).
- use db_read_bytes to get instructions
- move the address check logic previously attached only to fetching
instructions for disassembly to db_read_bytes (and db_write_bytes)
Motivated by related x86 changes this afternoon.
Note that the address check logic is not as sophisticated as what the
x86 code does, but it's what we had before. (Except that riscv will
now also try to fetch usermode instructions instead of just failing.)
(dholland)
diff -r1.42 -r1.43 src/sys/arch/mips/mips/db_disasm.c
diff -r1.93 -r1.94 src/sys/arch/mips/mips/db_interface.c
diff -r1.7 -r1.8 src/sys/arch/riscv/riscv/db_disasm.c
diff -r1.7 -r1.8 src/sys/arch/riscv/riscv/db_machdep.c
--- src/sys/arch/mips/mips/db_disasm.c 2021/04/12 11:35:22 1.42
+++ src/sys/arch/mips/mips/db_disasm.c 2021/05/23 23:22:55 1.43
@@ -1,4 +1,4 @@
-/* $NetBSD: db_disasm.c,v 1.42 2021/04/12 11:35:22 simonb Exp $ */
+/* $NetBSD: db_disasm.c,v 1.43 2021/05/23 23:22:55 dholland Exp $ */
/*-
* Copyright (c) 1991, 1993
@@ -35,7 +35,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: db_disasm.c,v 1.42 2021/04/12 11:35:22 simonb Exp $");
+__KERNEL_RCSID(0, "$NetBSD: db_disasm.c,v 1.43 2021/05/23 23:22:55 dholland Exp $");
#include <sys/param.h>
#include <sys/cpu.h>
@@ -47,6 +47,7 @@
#include <machine/db_machdep.h>
+#include <ddb/db_access.h>
#include <ddb/db_user.h>
#include <ddb/db_interface.h>
#include <ddb/db_output.h>
@@ -218,11 +219,6 @@
* "next instruction" does NOT mean the next instruction to
* be executed but the 'linear' next instruction.
*/
-#ifdef _LP64
-#define DISASM_KERN_START MIPS_XKSEG_START
-#else
-#define DISASM_KERN_START MIPS_KSEG0_START
-#endif
db_addr_t
db_disasm(db_addr_t loc, bool altfmt)
{
@@ -232,19 +228,14 @@
* Take some care with addresses to not UTLB here as it
* loses the current debugging context. KSEG2 and XKSEG
* are not checked.
+ * Update: db_read_bytes is supposed to do that, and now
+ * does, so we can use that.
+ *
+ * XXX db_read_bytes_can't return failure but instead zeros
+ * the output. That's ok here, but if ever improved the
+ * proper thing here on error is to return the original loc.
*/
- if (loc < (db_addr_t)DISASM_KERN_START) {
-#ifdef _KERNEL
- if (ufetch_32((void *)loc, &instr) != 0) {
- db_printf("invalid address.\n");
- return loc;
- }
-#else
- return loc;
-#endif
- } else {
- instr = *(uint32_t *)loc;
- }
+ db_read_bytes(loc, sizeof(instr), (void *)&instr);
return (db_disasm_insn(instr, loc, altfmt));
}
--- src/sys/arch/mips/mips/db_interface.c 2021/04/12 02:23:41 1.93
+++ src/sys/arch/mips/mips/db_interface.c 2021/05/23 23:22:55 1.94
@@ -1,4 +1,4 @@
-/* $NetBSD: db_interface.c,v 1.93 2021/04/12 02:23:41 mrg Exp $ */
+/* $NetBSD: db_interface.c,v 1.94 2021/05/23 23:22:55 dholland Exp $ */
/*
* Mach Operating System
@@ -27,7 +27,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: db_interface.c,v 1.93 2021/04/12 02:23:41 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: db_interface.c,v 1.94 2021/05/23 23:22:55 dholland Exp $");
#ifdef _KERNEL_OPT
#include "opt_multiprocessor.h"
@@ -180,7 +180,24 @@
db_read_bytes(vaddr_t addr, size_t size, char *data)
{
const char *src = (char *)addr;
+ int err;
+ /*
+ * If asked to fetch from userspace, do it safely.
+ * Note that MIPS_KSEG0_START is the proper address for
+ * both 32-bit and 64-bit kernels.
+ */
+ if (addr < (vaddr_t)MIPS_KSEG0_START) {
+ err = copyin(src, data, size);
+ if (err) {
+#ifdef DDB
+ db_printf("address %p is invalid\n", src);
+#endif
+ memset(data, 0, size);
+ }
+ return;
+ }
+
if (size <= 8 && (size & (size-1)) == 0 && (addr & (size-1)) == 0
&& ((uintptr_t)data & (size-1)) == 0) {
if (size == sizeof(uint8_t))
@@ -205,6 +222,18 @@
{
char *p = (char *)addr;
size_t n = size;
+ int err;
+
+ /* If asked to fetch from userspace, do it safely */
+ if (addr < (vaddr_t)MIPS_KSEG0_START) {
+ err = copyout(data, p, size);
+ if (err) {
+#ifdef DDB
+ db_printf("address %p is invalid\n", p);
+#endif
+ }
+ return;
+ }
if (size <= 8 && (size & (size-1)) == 0 && (addr & (size-1)) == 0
&& ((uintptr_t)data & (size-1)) == 0) {
--- src/sys/arch/riscv/riscv/db_disasm.c 2021/05/01 06:48:51 1.7
+++ src/sys/arch/riscv/riscv/db_disasm.c 2021/05/23 23:22:55 1.8
@@ -1,4 +1,4 @@
-/* $NetBSD: db_disasm.c,v 1.7 2021/05/01 06:48:51 skrll Exp $ */
+/* $NetBSD: db_disasm.c,v 1.8 2021/05/23 23:22:55 dholland Exp $ */
/*-
* Copyright (c) 2014 The NetBSD Foundation, Inc.
@@ -31,7 +31,7 @@
#include <sys/cdefs.h>
-__RCSID("$NetBSD: db_disasm.c,v 1.7 2021/05/01 06:48:51 skrll Exp $");
+__RCSID("$NetBSD: db_disasm.c,v 1.8 2021/05/23 23:22:55 dholland Exp $");
#include <sys/param.h>
#include <sys/systm.h>
@@ -39,6 +39,7 @@
#include <riscv/db_machdep.h>
#include <riscv/insn.h>
+#include <ddb/db_access.h>
#include <ddb/db_user.h>
#include <ddb/db_interface.h>
#include <ddb/db_output.h>
@@ -1463,23 +1464,16 @@
unsigned n, i;
uint32_t insn32;
-#ifdef _KERNEL
- if ((intptr_t) loc >= 0) {
- db_printf("%s: %#"PRIxVADDR" is not a kernel address\n",
- __func__, loc);
- return loc;
- }
-#endif
-
/*
* Fetch the instruction. The first halfword tells us how many
* more there are, and they're always in little-endian order.
*/
- insn[0] = ((const uint16_t *)loc)[0];
+ db_read_bytes(loc, sizeof(insn[0]), (void *)&insn[0]);
n = INSN_HALFWORDS(insn[0]);
KASSERT(n > 0 && n <= 5);
for (i = 1; i < n; i++) {
- insn[i] = ((const uint16_t *)loc)[i];
+ db_read_bytes(loc + i * sizeof(insn[i]), sizeof(insn[i]),
+ (void *)&insn[i]);
}
switch (n) {
--- src/sys/arch/riscv/riscv/db_machdep.c 2021/04/14 06:32:20 1.7
+++ src/sys/arch/riscv/riscv/db_machdep.c 2021/05/23 23:22:55 1.8
@@ -1,4 +1,4 @@
-/* $NetBSD: db_machdep.c,v 1.7 2021/04/14 06:32:20 dholland Exp $ */
+/* $NetBSD: db_machdep.c,v 1.8 2021/05/23 23:22:55 dholland Exp $ */
/*-
* Copyright (c) 2014 The NetBSD Foundation, Inc.
@@ -31,7 +31,7 @@
#include <sys/cdefs.h>
-__RCSID("$NetBSD: db_machdep.c,v 1.7 2021/04/14 06:32:20 dholland Exp $");
+__RCSID("$NetBSD: db_machdep.c,v 1.8 2021/05/23 23:22:55 dholland Exp $");
#include <sys/param.h>
@@ -42,6 +42,7 @@
#include <ddb/db_interface.h>
#include <ddb/db_extern.h>
#include <ddb/db_variables.h>
+#include <ddb/db_output.h>
int db_active = 0;
@@ -225,7 +226,20 @@
db_read_bytes(db_addr_t addr, size_t len, char *data)
{
const char *src = (char *)addr;
+ int err;
+ /* If asked to fetch from userspace, do it safely */
+ if ((intptr_t)addr >= 0) {
+ err = copyin(src, data, len);
+ if (err) {
+#ifdef DDB
+ db_printf("address %p is invalid\n", src);
+#endif
+ memset(data, 0, len);
+ }
+ return;
+ }
+
while (len--) {
*data++ = *src++;
}
@@ -237,6 +251,19 @@
void
db_write_bytes(vaddr_t addr, size_t len, const char *data)
{
+ int err;
+
+ /* If asked to fetch from userspace, do it safely */
+ if ((intptr_t)addr >= 0) {
+ err = copyout(data, (char *)addr, len);
+ if (err) {
+#ifdef DDB
+ db_printf("address %p is invalid\n", (char *)addr);
+#endif
+ }
+ return;
+ }
+
if (len == 8) {
*(uint64_t *)addr = *(const uint64_t *) data;
} else if (len == 4) {
@@ -244,6 +271,7 @@
} else if (len == 2) {
*(uint16_t *)addr = *(const uint16_t *) data;
} else {
+ KASSERT(len == 1);
*(uint8_t *)addr = *(const uint8_t *) data;
}
__asm("fence rw,rw; fence.i");