[v2] kprobes/x86: Use copy_from_kernel_nofault() to read from unsafe address

Commit Message

Masami Hiramatsu (Google) March 14, 2024, 3:17 p.m. UTC

From: Masami Hiramatsu (Google) <[e-mail]>

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.

Reported-by: Qiang Zhang <[e-mail]>
Closes: https://lore.kernel.org/all/[e-mail]/
Fixes: cc66bb914578 ("x86/ibt,kprobes: Cure sym+0 equals fentry woes")
Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
---
 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(-)

Comments

Jinghao Jia March 14, 2024, 11:56 p.m. UTC | #1

On 3/14/24 10:17, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[e-mail]>
> 
> 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.

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

Thanks,
--Jinghao

> 
> Reported-by: Qiang Zhang <[e-mail]>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/all/[e-mail]/__;!!DZ3fjg!_C9Dn6-GBlkyS2z34bDUBsEXkTkgWp45MNrd4Rl5I5slz2A3SrurXOxKupsxLMqxC2BMiySCTfB2-5fPhkLP1g$ 
> Fixes: cc66bb914578 ("x86/ibt,kprobes: Cure sym+0 equals fentry woes")
> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> ---
>  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;
>

Masami Hiramatsu (Google) March 16, 2024, 1:46 p.m. UTC | #2

On Thu, 14 Mar 2024 18:56:35 -0500
Jinghao Jia <[e-mail]> wrote:

> On 3/14/24 10:17, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[e-mail]>
> > 
> > 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.

Thank you,

> 
> Thanks,
> --Jinghao
> 
> > 
> > Reported-by: Qiang Zhang <[e-mail]>
> > Closes: https://urldefense.com/v3/__https://lore.kernel.org/all/[e-mail]/__;!!DZ3fjg!_C9Dn6-GBlkyS2z34bDUBsEXkTkgWp45MNrd4Rl5I5slz2A3SrurXOxKupsxLMqxC2BMiySCTfB2-5fPhkLP1g$ 
> > Fixes: cc66bb914578 ("x86/ibt,kprobes: Cure sym+0 equals fentry woes")
> > Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> > ---
> >  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;
> >

Jinghao Jia March 17, 2024, 3:53 p.m. UTC | #3

On 3/16/24 08:46, Masami Hiramatsu (Google) wrote:
> On Thu, 14 Mar 2024 18:56:35 -0500
> Jinghao Jia <[e-mail]> wrote:
> 
>> On 3/14/24 10:17, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <[e-mail]>
>>>
>>> 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 <[e-mail]>

--Jinghao

> 
> Thank you,
> 
>>
>> Thanks,
>> --Jinghao
>>
>>>
>>> Reported-by: Qiang Zhang <[e-mail]>
>>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/all/[e-mail]/__;!!DZ3fjg!_C9Dn6-GBlkyS2z34bDUBsEXkTkgWp45MNrd4Rl5I5slz2A3SrurXOxKupsxLMqxC2BMiySCTfB2-5fPhkLP1g$ 
>>> Fixes: cc66bb914578 ("x86/ibt,kprobes: Cure sym+0 equals fentry woes")
>>> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
>>> ---
>>>  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;
>>>
> 
>

Masami Hiramatsu (Google) March 20, 2024, 8:12 a.m. UTC | #4

On Sun, 17 Mar 2024 10:53:59 -0500
Jinghao Jia <[e-mail]> wrote:

> 
> 
> On 3/16/24 08:46, Masami Hiramatsu (Google) wrote:
> > On Thu, 14 Mar 2024 18:56:35 -0500
> > Jinghao Jia <[e-mail]> wrote:
> > 
> >> On 3/14/24 10:17, Masami Hiramatsu (Google) wrote:
> >>> From: Masami Hiramatsu (Google) <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> 

Thank you for testing!

Regards,

Patch

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;