linux-kernel - Re: [RFC PATCH 1/2] x86/kprobes: Prohibit kprobing on INT and UD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <15690bf4-a803-4ce0-87bc-ec21727fa38e@illinois.edu>
Date: Mon, 29 Jan 2024 20:50:39 -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/29/24 19:44, Masami Hiramatsu (Google) wrote:
> On Sun, 28 Jan 2024 15:25:59 -0600
> Jinghao Jia <jinghao7@...inois.edu> wrote:
> 
>>>>  /* 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

>>
>> --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)