Received-SPF: pass (google.com: domain of linux-kernel+bounces-108598-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 20 Mar 2024 17:12:03 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Jinghao Jia <jinghao7@illinois.edu>
Cc: Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>, LKML
 <linux-kernel@vger.kernel.org>, Steven Rostedt <rostedt@goodmis.org>, Qiang
 Zhang <zzqq0103.hey@gmail.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo
 Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen
 <dave.hansen@linux.intel.com>, "H . Peter Anvin" <hpa@zytor.com>, Peter
 Zijlstra <peterz@infradead.org>, x86@kernel.org
Subject: Re: [PATCH v2] kprobes/x86: Use copy_from_kernel_nofault() to read
 from unsafe address
Message-Id: <20240320171203.d493d214dea91a18114994cd@kernel.org>
In-Reply-To: <12453ce8-0b78-4c1c-9aca-de4cc366e3e1@illinois.edu>
References: <20240315000753.a448251fce0291e041f76c13@kernel.org>
	<171042945004.154897.2221804961882915806.stgit@devnote2>
	<fb6919c5-8acf-4ee3-8fd2-1d483b274867@illinois.edu>
	<20240316224630.01bd6b91938720f5083e0d07@kernel.org>
	<12453ce8-0b78-4c1c-9aca-de4cc366e3e1@illinois.edu>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 17 Mar 2024 10:53:59 -0500
Jinghao Jia <jinghao7@illinois.edu> wrote:

> 
> 
> On 3/16/24 08:46, Masami Hiramatsu (Google) wrote:
> > On Thu, 14 Mar 2024 18:56:35 -0500
> > Jinghao Jia <jinghao7@illinois.edu> wrote:
> > 
> >> On 3/14/24 10:17, Masami Hiramatsu (Google) wrote:
> >>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >>>
> >>> Read from an unsafe address with copy_from_kernel_nofault() in
> >>> arch_adjust_kprobe_addr() because this function is used before checking
> >>> the address is in text or not. Syzcaller bot found a bug and reported
> >>> the case if user specifies inaccessible data area,
> >>> arch_adjust_kprobe_addr() will cause a kernel panic.
> >>
> >> IMHO there is a check on the address in kallsyms_lookup_size_offset to see
> >> if it is a kernel text address before arch_adjust_kprobe_addr is invoked.
> > 
> > Yeah, kallsyms does not ensure the page (especially data) exists.
> > 
> >>
> >> The call chain is:
> >>
> >> register_kprobe()
> >>   _kprobe_addr()
> >>     kallsyms_lookup_size_offset() <- check on addr is here
> >>     arch_adjust_kprobe_addr()
> >>
> >> I wonder why this check was not able to capture the problem in this bug
> >> report (I cannot reproduce it locally).
> > 
> > I could reproduce it locally, it tried to access 'Y' data.
> > (I attached my .config) And I ensured that this fixed the problem.
> > 
> > The reproduce test actually tried to access initdata area
> > 
> > ffffffff82fb5450 d __alt_reloc_selftest_addr
> > ffffffff82fb5460 d int3_exception_nb.1
> > ffffffff82fb5478 d tsc_early_khz
> > ffffffff82fb547c d io_delay_override
> > ffffffff82fb5480 d fxregs.0
> > ffffffff82fb5680 d y                    <--- access this
> > ffffffff82fb5688 d x
> > ffffffff82fb56a0 d xsave_cpuid_features
> > ffffffff82fb56c8 d l1d_flush_mitigation
> > 
> > `y` is too generic, so check `io_delay_override` which is on the
> > same page.
> > 
> > $ git grep io_delay_override
> > arch/x86/kernel/io_delay.c:static int __initdata io_delay_override;
> > 
> > As you can see, it is marked as `__initdata`, and the initdata has been
> > freed before starting /init.
> > 
> > ----
> > [    2.679161] Freeing unused kernel image (initmem) memory: 2888K
> > [    2.688731] Write protecting the kernel read-only data: 24576k
> > [    2.691802] Freeing unused kernel image (rodata/data gap) memory: 1436K
> > [    2.746994] x86/mm: Checked W+X mappings: passed, no W+X pages found.
> > [    2.748022] x86/mm: Checking user space page tables
> > [    2.789520] x86/mm: Checked W+X mappings: passed, no W+X pages found.
> > [    2.790527] Run /init as init process
> > ----
> > 
> > So this has been caused because accessing freed initdata.
> 
> Thanks a lot for the explanation! I have confirmed the bug and tested the
> patch with CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y (which explicitly marks
> the init pages as not-present after boot).
> 
> Tested-by: Jinghao Jia <jinghao7@illinois.edu>
> 

Thank you for testing!

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