[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8c29d66d-b17d-4185-988c-de078566d0da@illinois.edu>
Date: Sun, 28 Jan 2024 15:25:59 -0600
From: Jinghao Jia <jinghao7@...inois.edu>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>,
Peter Zijlstra <peterz@...radead.org>,
linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] x86/kprobes: Prohibit kprobing on INT and UD
On 1/27/24 19:19, Masami Hiramatsu (Google) wrote:
> On Fri, 26 Jan 2024 22:41:23 -0600
> Jinghao Jia <jinghao7@...inois.edu> 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) <mhiramat@...nel.org>
>> Signed-off-by: Jinghao Jia <jinghao7@...inois.edu>
>> ---
>> 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
>>
>
>
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists