Received-SPF: pass (google.com: domain of linux-kernel+bounces-44512-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 30 Jan 2024 20:30:28 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Jinghao Jia <jinghao7@illinois.edu>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
 x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>, Peter Zijlstra
 <peterz@infradead.org>, linux-trace-kernel@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] x86/kprobes: Prohibit kprobing on INT and UD
Message-Id: <20240130203028.320df28af0bcaf9ce8c91d4d@kernel.org>
In-Reply-To: <15690bf4-a803-4ce0-87bc-ec21727fa38e@illinois.edu>
References: <20240127044124.57594-1-jinghao7@illinois.edu>
	<20240127044124.57594-2-jinghao7@illinois.edu>
	<20240128101912.5ad6717347bd66089ecea03a@kernel.org>
	<8c29d66d-b17d-4185-988c-de078566d0da@illinois.edu>
	<20240130104414.4548b70ada33ea3180047423@kernel.org>
	<15690bf4-a803-4ce0-87bc-ec21727fa38e@illinois.edu>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 29 Jan 2024 20:50:39 -0600
Jinghao Jia <jinghao7@illinois.edu> wrote:

> On 1/29/24 19:44, Masami Hiramatsu (Google) wrote:
> > On Sun, 28 Jan 2024 15:25:59 -0600
> > Jinghao Jia <jinghao7@illinois.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.

Yes, that is also a good change :)

Thank you,

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


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