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@xxxxxxxxxx>
> > ---
> >  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