[PATCH 0/2] Fix two bugs for profile

[PATCH 0/2] Fix two bugs for profile

[Chen Zhongjin]

Fixe two bugs reported by syzkaller for profile, see patch for detail.

Chen Zhongjin (2):
  x86: profiling: remove lock functions hack for !FRAME_POINTER
  profiling: Check prof_buffer in profile_tick()

 arch/x86/kernel/time.c | 14 +-------------
 kernel/profile.c       |  2 +-
 2 files changed, 2 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Chen Zhongjin]

Syzbot has been reporting the problem of stack-out-of-bounds in
profile_pc for a long time:
https://syzkaller.appspot.com/bug?extid=84fe685c02cd112a2ac3

profile_pc tries to get pc if current regs is inside lock function. For
!CONFIG_FRAME_POINTER it used a hack way to get the pc from stack, which
is not work with ORC. It makes profile_pc read illeagal address, return
wrong result, and frequently triggers KASAN.

Since lock profiling can be handled with much better other tools, It's
reasonable to remove lock functions hack for !FRAME_POINTER kernel.

Suggested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/x86/kernel/time.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index e42faa792c07..e08fac7bb71e 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -29,22 +29,10 @@ unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
 
-	if (!user_mode(regs) && in_lock_functions(pc)) {
 #ifdef CONFIG_FRAME_POINTER
+	if (!user_mode(regs) && in_lock_functions(pc))
 		return *(unsigned long *)(regs->bp + sizeof(long));
-#else
-		unsigned long *sp = (unsigned long *)regs->sp;
-		/*
-		 * Return address is either directly at stack pointer
-		 * or above a saved flags. Eflags has bits 22-31 zero,
-		 * kernel addresses don't.
-		 */
-		if (sp[0] >> 22)
-			return sp[0];
-		if (sp[1] >> 22)
-			return sp[1];
 #endif
-	}
 	return pc;
 }
 EXPORT_SYMBOL(profile_pc);
-- 
2.17.1

Re: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Dave Hansen]

On 4/9/23 19:22, Chen Zhongjin wrote:
> Syzbot has been reporting the problem of stack-out-of-bounds in
> profile_pc for a long time:
> https://syzkaller.appspot.com/bug?extid=84fe685c02cd112a2ac3
> 
> profile_pc tries to get pc if current regs is inside lock function. For
> !CONFIG_FRAME_POINTER it used a hack way to get the pc from stack, which
> is not work with ORC. It makes profile_pc read illeagal address, return
> wrong result, and frequently triggers KASAN.
> 
> Since lock profiling can be handled with much better other tools, It's
> reasonable to remove lock functions hack for !FRAME_POINTER kernel.

OK, so let me make sure I understand what's going on:

1. This whole issue is limited to kernel/profile.c which is what drives
   readprofile(8) and /proc/profile
2. This is removing code that got added in 2006:
	0cb91a229364 ("[PATCH] i386: Account spinlocks to the caller during
profiling for !FP kernels")
3. This was an OK hack back in the day, but it outright breaks today
   in some situations.  KASAN also didn't exist in 2006.
4. !CONFIG_FRAME_POINTER is probably even more rare today than it was in
   2006
5. Lock function caller information is available at _least_ from perf,
   maybe other places too??  (What "much better other tools" are there?)

Given all that, this patch suggests that we can remove the stack peeking
hack.  The downside is that /proc/profile users will see their profiles
pointing to the spinlock functions like they did in 2005.  The upside is
that we won't get any more KASAN reports.

If anyone complains, I assume we're just going to tell them to run 'perf
--call-graph' and to go away (which also probably didn't exist in 2006).

If I got all that right, the end result seems sane to me.  It would be
_nice_ if you could make a more coherent changelog out of that and
resend.  Also, considering that your two "profile" issues are quite
independent, you can probably just resend the two patches separately.

Re: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Chen Zhongjin]

On 2023/4/11 3:34, Dave Hansen wrote:
> On 4/9/23 19:22, Chen Zhongjin wrote:
>> Syzbot has been reporting the problem of stack-out-of-bounds in
>> profile_pc for a long time:
>> https://syzkaller.appspot.com/bug?extid=84fe685c02cd112a2ac3
>>
>> profile_pc tries to get pc if current regs is inside lock function. For
>> !CONFIG_FRAME_POINTER it used a hack way to get the pc from stack, which
>> is not work with ORC. It makes profile_pc read illeagal address, return
>> wrong result, and frequently triggers KASAN.
>>
>> Since lock profiling can be handled with much better other tools, It's
>> reasonable to remove lock functions hack for !FRAME_POINTER kernel.
> OK, so let me make sure I understand what's going on:
>
> 1. This whole issue is limited to kernel/profile.c which is what drives
>     readprofile(8) and /proc/profile
> 2. This is removing code that got added in 2006:
> 	0cb91a229364 ("[PATCH] i386: Account spinlocks to the caller during
> profiling for !FP kernels")
> 3. This was an OK hack back in the day, but it outright breaks today
>     in some situations.  KASAN also didn't exist in 2006.
Yes, and whether KASAN is enabled it can make problem.
Some lock_function will save registers on stack (this may not happen in 
2006).
These registers can be recorded by profile and be read outside of kernel,
which is risky theoretically.
> 4. !CONFIG_FRAME_POINTER is probably even more rare today than it was in
>     2006
No. !CONFIG_FRAME_POINTER is more common today because of UNWINDER_ORC.
And that is why the bug is triggered more frequently.
> 5. Lock function caller information is available at _least_ from perf,
>     maybe other places too??  (What "much better other tools" are there?)

Yes, it's basically about perf function graph.

> Given all that, this patch suggests that we can remove the stack peeking
> hack.  The downside is that /proc/profile users will see their profiles
> pointing to the spinlock functions like they did in 2005.  The upside is
> that we won't get any more KASAN reports.
>
> If anyone complains, I assume we're just going to tell them to run 'perf
> --call-graph' and to go away (which also probably didn't exist in 2006).
>
> If I got all that right, the end result seems sane to me.  It would be
> _nice_ if you could make a more coherent changelog out of that and
> resend.  Also, considering that your two "profile" issues are quite
> independent, you can probably just resend the two patches separately.
Thanks for review and I'll send another version to provide better details.

RE: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[David Laight]

From: Chen Zhongjin
> Sent: 12 April 2023 08:02
...
> > 4. !CONFIG_FRAME_POINTER is probably even more rare today than it was in
> >     2006
>
> No. !CONFIG_FRAME_POINTER is more common today because of UNWINDER_ORC.
> And that is why the bug is triggered more frequently.

CONFIG_FRAME_POINTER is pretty much required (on x86-64)
for the user copy checks that are enabled in distro kernels.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Chen Zhongjin]

On 2023/4/12 18:01, David Laight wrote:
> From: Chen Zhongjin
>> Sent: 12 April 2023 08:02
> ...
>>> 4. !CONFIG_FRAME_POINTER is probably even more rare today than it was in
>>>      2006
>> No. !CONFIG_FRAME_POINTER is more common today because of UNWINDER_ORC.
>> And that is why the bug is triggered more frequently.
> CONFIG_FRAME_POINTER is pretty much required (on x86-64)
> for the user copy checks that are enabled in distro kernels.

Thanks for reminding.. I know little about distro kernels.

I though at least !CONFIG_FRAME_POINTER is more common than it was in 2006.
Not investigated but just because UNWINDER_ORC is defconfig and it 
disables CONFIG_FRAME_POINTER.

Anyway, I kept the CONFIG_FRAME_POINTER not removed so it won't be affected.

Best,
Chen
> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Josh Poimboeuf]

On Wed, Apr 12, 2023 at 10:01:18AM +0000, David Laight wrote:
> From: Chen Zhongjin
> > Sent: 12 April 2023 08:02
> ...
> > > 4. !CONFIG_FRAME_POINTER is probably even more rare today than it was in
> > >     2006
> >
> > No. !CONFIG_FRAME_POINTER is more common today because of UNWINDER_ORC.
> > And that is why the bug is triggered more frequently.
> 
> CONFIG_FRAME_POINTER is pretty much required (on x86-64)
> for the user copy checks that are enabled in distro kernels.

CONFIG_FRAME_POINTER is pretty much deprecated.  The only distro still
using it (that I know of) is Ubuntu.

-- 
Josh

Re: [PATCH 1/2] x86: profiling: remove lock functions hack for !FRAME_POINTER

[Josh Poimboeuf]

On Mon, Apr 10, 2023 at 10:22:25AM +0800, Chen Zhongjin wrote:
> Syzbot has been reporting the problem of stack-out-of-bounds in
> profile_pc for a long time:
> https://syzkaller.appspot.com/bug?extid=84fe685c02cd112a2ac3
> 
> profile_pc tries to get pc if current regs is inside lock function. For
> !CONFIG_FRAME_POINTER it used a hack way to get the pc from stack, which
> is not work with ORC. It makes profile_pc read illeagal address, return
> wrong result, and frequently triggers KASAN.
> 
> Since lock profiling can be handled with much better other tools, It's
> reasonable to remove lock functions hack for !FRAME_POINTER kernel.
> 
> Suggested-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/x86/kernel/time.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
> index e42faa792c07..e08fac7bb71e 100644
> --- a/arch/x86/kernel/time.c
> +++ b/arch/x86/kernel/time.c
> @@ -29,22 +29,10 @@ unsigned long profile_pc(struct pt_regs *regs)
>  {
>  	unsigned long pc = instruction_pointer(regs);
>  
> -	if (!user_mode(regs) && in_lock_functions(pc)) {
>  #ifdef CONFIG_FRAME_POINTER
> +	if (!user_mode(regs) && in_lock_functions(pc))

If lock profiling is no longer useful then we should just remove it
altogether, not just for ORC.

-- 
Josh

[PATCH 2/2] profiling: Check prof_buffer in profile_tick()

[Chen Zhongjin]

KASAN reported a UAF problem in profile_tick():

  BUG: KASAN: use-after-free in profile_tick+0x5c/0x80
  Read of size 8 at addr ffff888100928aa0 by task bash/1108

  CPU: 2 PID: 1108 Comm: bash Not tainted 5.10.0+ #72
  Call Trace:
   <IRQ>
   dump_stack+0x93/0xc5
   print_address_description.constprop.0+0x1c/0x3c0
   kasan_report.cold+0x37/0x74
   check_memory_region+0x161/0x1c0
   profile_tick+0x5c/0x80
   tick_sched_timer+0xcd/0x100
   __hrtimer_run_queues+0x23e/0x480
   hrtimer_interrupt+0x1c2/0x440
   asm_call_irq_on_stack+0xf/0x20
   </IRQ>
  ...

It is beacause in profiling_store(), profile_init() is possible to fail
and free prof_cpu_mask. However prof_cpu_mask is not set to NULL and
cpumask_available(prof_cpu_mask) will return true in profile_tick().
Then cpumask_test_cpu() will dereference prof_cpu_mask and trigger the
KASAN warning.

There is no interface to disable profile_tick() even though profile_init
has been already failed. And when CONFIG_CPUMASK_OFFSTACK=n, cpumask_var_t
is an array pointer which can not be set to NULL. So prof_cpu_mask can be
a freed pointner or uncleaned array here, and cpumask_available() can't
promise it is safe to use it.

To fix this, add check for prof_buffer in profile_tick() before check
cpumask_available(prof_cpu_mask). Because prof_cpu_mask is available only
after prof_buffer is allocated successfully.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 kernel/profile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/profile.c b/kernel/profile.c
index 8a77769bc4b4..cca47dc2b1db 100644
--- a/kernel/profile.c
+++ b/kernel/profile.c
@@ -334,7 +334,7 @@ void profile_tick(int type)
 {
 	struct pt_regs *regs = get_irq_regs();
 
-	if (!user_mode(regs) && cpumask_available(prof_cpu_mask) &&
+	if (!user_mode(regs) && prof_buffer && cpumask_available(prof_cpu_mask) &&
 	    cpumask_test_cpu(smp_processor_id(), prof_cpu_mask))
 		profile_hit(type, (void *)profile_pc(regs));
 }
-- 
2.17.1