On 1/27/24 19:19, Masami Hiramatsu (Google) wrote: > On Fri, 26 Jan 2024 22:41:23 -0600 > Jinghao Jia <[email protected]> wrote: > >> Both INTs (INT n, INT1, INT3, INTO) and UDs (UD0, UD1, UD2) serve >> special purposes in the kernel, e.g., INT3 is used by KGDB and UD2 is >> involved in LLVM-KCFI instrumentation. At the same time, attaching >> kprobes on these instructions (particularly UDs) will pollute the stack >> trace dumped in the kernel ring buffer, since the exception is triggered >> in the copy buffer rather than the original location. >> >> Check for INTs and UDs in can_probe and reject any kprobes trying to >> attach to these instructions. >> > > Thanks for implement this check! >
You are very welcome :) > >> Suggested-by: Masami Hiramatsu (Google) <[email protected]> >> Signed-off-by: Jinghao Jia <[email protected]> >> --- >> arch/x86/kernel/kprobes/core.c | 33 ++++++++++++++++++++++++++------- >> 1 file changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c >> index e8babebad7b8..792b38d22126 100644 >> --- a/arch/x86/kernel/kprobes/core.c >> +++ b/arch/x86/kernel/kprobes/core.c >> @@ -252,6 +252,22 @@ unsigned long >> recover_probed_instruction(kprobe_opcode_t *buf, unsigned long add >> return __recover_probed_insn(buf, addr); >> } >> >> +static inline int is_exception_insn(struct insn *insn) >> +{ >> + if (insn->opcode.bytes[0] == 0x0f) { >> + /* UD0 / UD1 / UD2 */ >> + return insn->opcode.bytes[1] == 0xff || >> + insn->opcode.bytes[1] == 0xb9 || >> + insn->opcode.bytes[1] == 0x0b; >> + } else { > > If "else" block just return, you don't need this "else". > > bool func() > { > if (cond) > return ... > > return ... > } > > Is preferrable because this puts "return val" always at the end of non-void > function. > I will fix this in the v2. >> + /* INT3 / INT n / INTO / INT1 */ >> + return insn->opcode.bytes[0] == 0xcc || >> + insn->opcode.bytes[0] == 0xcd || >> + insn->opcode.bytes[0] == 0xce || >> + insn->opcode.bytes[0] == 0xf1; >> + } >> +} >> + >> /* Check if paddr is at an instruction boundary */ >> static int can_probe(unsigned long paddr) >> { >> @@ -294,6 +310,16 @@ static int can_probe(unsigned long paddr) >> #endif >> addr += insn.length; >> } >> + __addr = recover_probed_instruction(buf, addr); >> + if (!__addr) >> + return 0; >> + >> + if (insn_decode_kernel(&insn, (void *)__addr) < 0) >> + return 0; >> + >> + if (is_exception_insn(&insn)) >> + return 0; >> + > > Please don't put this outside of decoding loop. You should put these in > the loop which decodes the instruction from the beginning of the function. > Since the x86 instrcution is variable length, can_probe() needs to check > whether that the address is instruction boundary and decodable. > > Thank you, If my understanding is correct then this is trying to decode the kprobe target instruction, given that it is after the main decoding loop. Here I hoisted the decoding logic out of the if(IS_ENABLED(CONFIG_CFI_CLANG)) block so that we do not need to decode the same instruction twice. I left the main decoding loop unchanged so it is still decoding the function from the start and should handle instruction boundaries. Are there any caveats that I missed? --Jinghao > >> if (IS_ENABLED(CONFIG_CFI_CLANG)) { >> /* >> * The compiler generates the following instruction sequence >> @@ -308,13 +334,6 @@ static int can_probe(unsigned long paddr) >> * Also, these movl and addl are used for showing expected >> * type. So those must not be touched. >> */ >> - __addr = recover_probed_instruction(buf, addr); >> - if (!__addr) >> - return 0; >> - >> - if (insn_decode_kernel(&insn, (void *)__addr) < 0) >> - return 0; >> - >> if (insn.opcode.value == 0xBA) >> offset = 12; >> else if (insn.opcode.value == 0x3) >> -- >> 2.43.0 >> > >
OpenPGP_signature.asc
Description: OpenPGP digital signature
