On 3/16/24 08:46, Masami Hiramatsu (Google) wrote: > On Thu, 14 Mar 2024 18:56:35 -0500 > Jinghao Jia <jingh...@illinois.edu> wrote: > >> On 3/14/24 10:17, Masami Hiramatsu (Google) wrote: >>> From: Masami Hiramatsu (Google) <mhira...@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 <jingh...@illinois.edu> --Jinghao > > Thank you, > >> >> Thanks, >> --Jinghao >> >>> >>> Reported-by: Qiang Zhang <zzqq0103....@gmail.com> >>> Closes: >>> https://urldefense.com/v3/__https://lore.kernel.org/all/cakhosas2rof6vqvbw_lg_j3qnku0canzr2qmy4et7r5lo8m...@mail.gmail.com/__;!!DZ3fjg!_C9Dn6-GBlkyS2z34bDUBsEXkTkgWp45MNrd4Rl5I5slz2A3SrurXOxKupsxLMqxC2BMiySCTfB2-5fPhkLP1g$ >>> >>> Fixes: cc66bb914578 ("x86/ibt,kprobes: Cure sym+0 equals fentry woes") >>> Signed-off-by: Masami Hiramatsu (Google) <mhira...@kernel.org> >>> --- >>> Changes in v2: >>> - Fix to return NULL if failed to access the address. >>> --- >>> arch/x86/kernel/kprobes/core.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c >>> index a0ce46c0a2d8..95e4ebe5d514 100644 >>> --- a/arch/x86/kernel/kprobes/core.c >>> +++ b/arch/x86/kernel/kprobes/core.c >>> @@ -335,7 +335,16 @@ static int can_probe(unsigned long paddr) >>> kprobe_opcode_t *arch_adjust_kprobe_addr(unsigned long addr, unsigned long >>> offset, >>> bool *on_func_entry) >>> { >>> - if (is_endbr(*(u32 *)addr)) { >>> + u32 insn; >>> + >>> + /* >>> + * Since addr is not guaranteed to be safely accessed yet, use >>> + * copy_from_kernel_nofault() to get the instruction. >>> + */ >>> + if (copy_from_kernel_nofault(&insn, (void *)addr, sizeof(u32))) >>> + return NULL; >>> + >>> + if (is_endbr(insn)) { >>> *on_func_entry = !offset || offset == 4; >>> if (*on_func_entry) >>> offset = 4; >>> > >
OpenPGP_signature.asc
Description: OpenPGP digital signature
