[PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[Will McVicker]

When the kobj->ktype is null, sysfs_file_ops() returns a NULL pointer
for the sysfs_ops. When this happens, we hit a kernel panic in
sysfs_kf_seq_show() by dereferencing ops to check if ->show is NULL.
Based on commit 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show"), it
sounds like we won't hit this often, but I have randomly hit this on my
Pixel 6 with 5.19-rc1. Refer to the crash stack below:

   Unable to handle kernel paging request at virtual address ...
   Internal error: Oops: 96000004 [#1] PREEMPT SMP
   Hardware name: Oriole EVT 1.0 (DT)
   pc : sysfs_kf_seq_show+0x3c/0x160
   lr : kernfs_seq_show+0x54/0xa0
   Call trace:
    sysfs_kf_seq_show+0x3c/0x160
    kernfs_seq_show+0x54/0xa0
    seq_read_iter+0x17c/0x638
    kernfs_fop_read_iter+0x70/0x1f4
    vfs_read+0x240/0x36c
    ksys_read+0x7c/0xf0
    __arm64_sys_read+0x20/0x30
    invoke_syscall+0x60/0x150
    el0_svc_common+0xb8/0x100
    do_el0_svc+0x30/0xd4
    el0_svc+0x30/0xc0
    el0t_64_sync_handler+0x88/0xf8
    el0t_64_sync+0x1a0/0x1a4

Fixes: 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show")
Signed-off-by: Will McVicker <willmcvicker@google.com>
---
 fs/sysfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index a12ac0356c69..f09f86f10410 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -45,7 +45,7 @@ static int sysfs_kf_seq_show(struct seq_file *sf, void *v)
 	ssize_t count;
 	char *buf;
 
-	if (WARN_ON_ONCE(!ops->show))
+	if (WARN_ON_ONCE(!ops || !ops->show))
 		return -EINVAL;
 
 	/* acquire buffer and ensure that it's >= PAGE_SIZE and clear */
-- 
2.36.1.476.g0c4daa206d-goog

Re: [PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[Greg Kroah-Hartman]

On Tue, Jun 14, 2022 at 05:24:01PM +0000, Will McVicker wrote:
> When the kobj->ktype is null,

How can that happen?  What in-tree code does that?

> sysfs_file_ops() returns a NULL pointer
> for the sysfs_ops. When this happens, we hit a kernel panic in
> sysfs_kf_seq_show() by dereferencing ops to check if ->show is NULL.
> Based on commit 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show"), it
> sounds like we won't hit this often, but I have randomly hit this on my
> Pixel 6 with 5.19-rc1. Refer to the crash stack below:
> 
>    Unable to handle kernel paging request at virtual address ...
>    Internal error: Oops: 96000004 [#1] PREEMPT SMP
>    Hardware name: Oriole EVT 1.0 (DT)
>    pc : sysfs_kf_seq_show+0x3c/0x160
>    lr : kernfs_seq_show+0x54/0xa0
>    Call trace:
>     sysfs_kf_seq_show+0x3c/0x160
>     kernfs_seq_show+0x54/0xa0
>     seq_read_iter+0x17c/0x638
>     kernfs_fop_read_iter+0x70/0x1f4
>     vfs_read+0x240/0x36c
>     ksys_read+0x7c/0xf0
>     __arm64_sys_read+0x20/0x30
>     invoke_syscall+0x60/0x150
>     el0_svc_common+0xb8/0x100
>     do_el0_svc+0x30/0xd4
>     el0_svc+0x30/0xc0
>     el0t_64_sync_handler+0x88/0xf8
>     el0t_64_sync+0x1a0/0x1a4
> 
> Fixes: 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show")
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  fs/sysfs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index a12ac0356c69..f09f86f10410 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -45,7 +45,7 @@ static int sysfs_kf_seq_show(struct seq_file *sf, void *v)
>  	ssize_t count;
>  	char *buf;
>  
> -	if (WARN_ON_ONCE(!ops->show))
> +	if (WARN_ON_ONCE(!ops || !ops->show))
>  		return -EINVAL;

Seems reasonable, but I want to track down how ops can be NULL here
under normal operation.

thanks,

greg k-h

Re: [PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[Greg Kroah-Hartman]

On Tue, Jun 14, 2022 at 11:38:27AM -0700, Will McVicker wrote:
> On Tue, Jun 14, 2022 at 10:31 AM Greg Kroah-Hartman <
> gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jun 14, 2022 at 05:24:01PM +0000, Will McVicker wrote:
> > > When the kobj->ktype is null,
> >
> > How can that happen?  What in-tree code does that?
> 
> This kernel panic happens randomly for me. The call trace shows that this
> happens when the read syscall is invoked by Android. GDB gave me this line
> when disassembling __arm64_sys_read+0x20/0x30:
> 
> fs/read_write.c:628
> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)

What sysfs file is it reading?  Any hint about that?

thanks,

greg k-h

Re: [PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[William McVicker]

On 06/14/2022, Greg Kroah-Hartman wrote:
> On Tue, Jun 14, 2022 at 05:24:01PM +0000, Will McVicker wrote:
> > When the kobj->ktype is null,
> 
> How can that happen?  What in-tree code does that?

This kernel panic happens randomly for me. The call trace shows that this
happens when the read syscall is invoked by Android. GDB gave me this line when
disassembling __arm64_sys_read+0x20/0x30:

fs/read_write.c:628
SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)

> 
> > sysfs_file_ops() returns a NULL pointer
> > for the sysfs_ops. When this happens, we hit a kernel panic in
> > sysfs_kf_seq_show() by dereferencing ops to check if ->show is NULL.
> > Based on commit 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show"), it
> > sounds like we won't hit this often, but I have randomly hit this on my
> > Pixel 6 with 5.19-rc1. Refer to the crash stack below:
> > 
> >    Unable to handle kernel paging request at virtual address ...
> >    Internal error: Oops: 96000004 [#1] PREEMPT SMP
> >    Hardware name: Oriole EVT 1.0 (DT)
> >    pc : sysfs_kf_seq_show+0x3c/0x160
> >    lr : kernfs_seq_show+0x54/0xa0
> >    Call trace:
> >     sysfs_kf_seq_show+0x3c/0x160
> >     kernfs_seq_show+0x54/0xa0
> >     seq_read_iter+0x17c/0x638
> >     kernfs_fop_read_iter+0x70/0x1f4
> >     vfs_read+0x240/0x36c
> >     ksys_read+0x7c/0xf0
> >     __arm64_sys_read+0x20/0x30
> >     invoke_syscall+0x60/0x150
> >     el0_svc_common+0xb8/0x100
> >     do_el0_svc+0x30/0xd4
> >     el0_svc+0x30/0xc0
> >     el0t_64_sync_handler+0x88/0xf8
> >     el0t_64_sync+0x1a0/0x1a4
> > 
> > Fixes: 820879ee1865 ("sysfs: simplify sysfs_kf_seq_show")
> > Signed-off-by: Will McVicker <willmcvicker@google.com>
> > ---
> >  fs/sysfs/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index a12ac0356c69..f09f86f10410 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -45,7 +45,7 @@ static int sysfs_kf_seq_show(struct seq_file *sf, void *v)
> >  	ssize_t count;
> >  	char *buf;
> >  
> > -	if (WARN_ON_ONCE(!ops->show))
> > +	if (WARN_ON_ONCE(!ops || !ops->show))
> >  		return -EINVAL;
> 
> Seems reasonable, but I want to track down how ops can be NULL here
> under normal operation.

Let me try to follow the code path through the call trace to see if I can track
down how ops could be NULL. It appears we could have hit this before commit
820879ee1865 ("sysfs: simplify sysfs_kf_seq_show") as well.

> 
> thanks,
> 
> greg k-h

Re: [PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 07:28:31PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 14, 2022 at 05:24:01PM +0000, Will McVicker wrote:
> > When the kobj->ktype is null,
> 
> How can that happen?  What in-tree code does that?

Yes, I'd be really curious how we arrived there.  I we ever end in
this case we're having a major problem, as all the sysfs files
should go through sysfs_add_file_mode_ns, which already derferences
kobj->ktype->sysfs_ops directly.  I.e. for this to happen
kobj->ktype must have been cleared on a live file, or someone
must have bypassed sysfs_add_file_mode_ns.

Re: [PATCH v1] sysfs: fix sysfs_kf_seq_show null pointer dereference

[William McVicker]

On 06/15/2022, Christoph Hellwig wrote:
> On Tue, Jun 14, 2022 at 07:28:31PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 14, 2022 at 05:24:01PM +0000, Will McVicker wrote:
> > > When the kobj->ktype is null,
> > 
> > How can that happen?  What in-tree code does that?
> 
> Yes, I'd be really curious how we arrived there.  I we ever end in
> this case we're having a major problem, as all the sysfs files
> should go through sysfs_add_file_mode_ns, which already derferences
> kobj->ktype->sysfs_ops directly.  I.e. for this to happen
> kobj->ktype must have been cleared on a live file, or someone
> must have bypassed sysfs_add_file_mode_ns.

Okay, so I was able to figure out that the Android userspace process that
triggers this issue is called rebalance_interrupts. You can find the source
code here [1]. I can reproduce this issue in about 5-10 reboots. As the name
indicates, it rebalances the IRQs. I found that the crash happens when the
program reads the sysfs files: /sys/kernel/irq/<irq>/actions. I haven't looked
into how kobj->ktype becomes null yet. I'll look deeper into that now, but
wanted to update this thread in case this information triggers any hints for
you guys on why this is happening.

Thanks,
Will

[1] https://android.googlesource.com/platform/hardware/google/pixel/+/refs/heads/android12-qpr3-s2-release/rebalance_interrupts