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]

Date: Tue, 30 Jan 2024 10:44:14 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Jinghao Jia <jinghao7@...inois.edu>
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 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,

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


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>