Change in coreboot[master]: arch/x86: Add support for catching null dereferences through debug regs
Attention is currently required from: Nico Huber, Raul Rangel, Tim Wawrzynczak, Paul Menzel, Angel Pons, Arthur Heymans, Kyösti Mälkki, Karthik Ramasubramanian. Robert Zieba has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63657 )
Change subject: arch/x86: Add support for catching null dereferences through debug regs ......................................................................
Patch Set 7:
(25 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63657/comment/66ec606e_bd569c51 PS5, Line 7: deferences
typo: de*re*ferences
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/83479cc3_046def63 PS5, Line 9: deferences
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/07324694_2991fbf4 PS5, Line 12: have been
Present tense: are
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/d0a62789_00e66afe PS5, Line 18: expected
How can this be tested exactly? Does it also work in QEMU?
Reading/writing at address zero or jumping to address 0 will trigger it. Debug registers are part of the x86 architecture so running in QEMU will work.
Patchset:
PS5:
Can you please make sure, it also builds with clang?
Done
File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/63657/comment/904654be_63ece18c PS5, Line 327:
Nit: Replace 2nd tab with 2 spaces so that it is consistent with other config items.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/26a91b32_11ebf786 PS5, Line 327: Enables
Imperative mood seems to be used in this file: Enable.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/6a746125_7d41e91b PS5, Line 335: Enables
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/72f58c42_cca71818 PS5, Line 335: deferences
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/a472dcca_be9180aa PS5, Line 342: deferences
Ditto.
Done
File src/arch/x86/breakpoint.c:
PS5:
indent this whole file with tabs please,
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/c52ec733_c43c98fe PS5, Line 35: #define DEBUG_CTRL_INSTR 0x0 : #define DEBUG_CTRL_WRITE 0x1 : #define DEBUG_CTRL_IO_RW 0x2 : #define DEBUG_CTRL_RW 0x3
enum seems like a good candidate
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/1eec664d_7672e918 PS5, Line 50: {
Nit: For functions, recommend to keep opening braces in a separate line.
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/15859f0d_78e66a89 PS5, Line 51: uintptr_t ret = ~0x0;
Nit: Always leave an empty line between a declaration block and function body. […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/b172d1d4_2b5b86f1 PS5, Line 173: case 8:
Nit: […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/fa330397_d938c8c6 PS5, Line 280: #if ENV_X86_64
Is it possible to check this in C code?
It's not possible because rflags is only defined for 64-bit code.
File src/arch/x86/exception.c:
https://review.coreboot.org/c/coreboot/+/63657/comment/d5801bb6_36ea3c0f PS5, Line 374: #include <arch/registers.h>
If this source file has been using anything from registers. […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/52dd44a0_d83ae08d PS5, Line 491: #if ENV_RAMSTAGE && CONFIG(DEBUG_HW_BREAKPOINTS)
If you have a static inline stubbed out implementation of breakpoint_dispatch_handler in breakpoint. […]
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/fc8694d9_4f9e509a PS5, Line 492: 1
What is this magic number 1?
Done
File src/arch/x86/include/arch/breakpoint.h:
https://review.coreboot.org/c/coreboot/+/63657/comment/f0a35a3c_71161e99 PS5, Line 5: CONFIG(DEBUG_HW_BREAKPOINTS)
I dont think this is required since it is included only in breakpoint.c and null_breakpoint. […]
It's also included in exception.c.
https://review.coreboot.org/c/coreboot/+/63657/comment/bcd7482c_6e74197d PS5, Line 13: BREAKPOINT_RES_OK = 0, : BREAKPOINT_RES_NONE_AVAILABLE = -1, : BREAKPOINT_RES_INVALID_HANDLE = -2, : BREAKPOINT_RES_INVALID_LENGTH = -3 : }; : : enum breakpoint_type { : BREAKPOINT_TYPE_INSTRUCTION, : BREAKPOINT_TYPE_DATA, : BREAKPOINT_TYPE_IO,
indent with tabs please
Done
File src/arch/x86/include/arch/null_breakpoint.h:
https://review.coreboot.org/c/coreboot/+/63657/comment/ae0ea8af_a2da74d9 PS5, Line 5: #if CONFIG(DEBUG_NULL_DEREF_BREAKPOINTS)
I dont think this is required. This is included only in null_breakpoint. […]
It's also included in exception.c. I've updated the file to declare `null_breakpoint_init` as a static inline stub when the proper config isn't set.
File src/arch/x86/null_breakpoint.c:
PS5:
indent with tabs
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/50644c49_57bfc888 PS5, Line 12: if (handle == null_exec_bp) { : printk(BIOS_ERR, "Null instruction execution\n"); : } : else { : #if ENV_X86_64 : printk(BIOS_ERR, "Null dereference at rip: 0x%llx \n", regs->rip); : #else : printk(BIOS_ERR, "Null dereference at eip: 0x%x \n", regs->eip); : #endif : }
Done
https://review.coreboot.org/c/coreboot/+/63657/comment/a406a318_b1ed29ff PS5, Line 29: if (res != BREAKPOINT_RES_OK) { : printk(BIOS_ERR, "Failed to create NULL dereference breakpoint\n"); : } : : res = breakpoint_create_instruction(&null_exec_bp, NULL); : if (res != BREAKPOINT_RES_OK) { : printk(BIOS_ERR, "Failed to create NULL execution breakpoint\n"); : }
braces not required for single-statement following an if
Done
-
Robert Zieba (Code Review)