Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 20 Mar 2024 17:12:03 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [PATCH v2] kprobes/x86: Use copy_from_kernel_nofault() to read from unsafe address |
| |
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>
| ![\](/images/icornerr.gif) |