On 1/29/24 19:44, Masami Hiramatsu (Google) wrote:

On Sun, 28 Jan 2024 15:25:59 -0600
Jinghao Jia [off-list ref] wrote:

quoted

quoted

quoted

 /* 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?

Ah, sorry I misread the patch. You're correct!
This is a good place to do that.

But hmm, I think we should add another patch to check the addr == paddr
soon after the loop so that we will avoid decoding.

Thank you,

Yes, that makes sense to me. At the same time, I'm also thinking about
changing the return type of can_probe() to bool, since we are just using
int as bool in this context.

--Jinghao

quoted

--Jinghao

quoted

quoted

 	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