Re: Re: [PATCH] debugfs: Fix NULL pointer dereference at debugfs_read_file_str

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 22, 2025 at 08:41:33PM +0800, yangshiguang wrote:
> 
> At 2025-12-22 19:54:22, "Greg KH" <gregkh@linuxfoundation.org> wrote:
> >On Mon, Dec 22, 2025 at 05:36:16PM +0800, yangshiguang1011@163.com wrote:
> >> From: yangshiguang <yangshiguang@xiaomi.com>
> >> 
> >> Check in debugfs_read_file_str() if the string pointer is NULL.
> >> 
> >> When creating a node using debugfs_create_str(), the string parameter
> >> value can be NULL to indicate empty/unused/ignored.
> >
> >Why would you create an empty debugfs string file?  That is not ok, we
> >should change that to not allow this.
> 
> Hi greg k-h,
> 
> Thanks for your reply.
> 
> This is due to the usage step, should write first and then read.
> However, there is no way to guarantee that everyone will know about this step.

True.

> And debugfs_create_str() allows passing in a NULL string. 

Then we should fix that :)

> Therefore, when reading a NULL string, should return an invalid error 
> instead of panic.

If you call write on a NULL string, then you could call strlen() of that
NULL string, and do a memcpy out of that NULL string.  All not good
things, so your quick fix here really doesn't solve the root problem :(

> >>  	str = *(char **)file->private_data;
> >> +	if (!str)
> >> +		return -EINVAL;
> >
> >What in kernel user causes this to happen?  Let's fix that up instead
> >please.
> >
> 
> Currently I known problematic nodes in the kernel:
> 
> drivers/interconnect/debugfs-client.c:
>   155: 	debugfs_create_str("src_node", 0600, client_dir, &src_node);
>   156: 	debugfs_create_str("dst_node", 0600, client_dir, &dst_node);

Ick, ok, that should be fixed.

> drivers/soundwire/debugfs.c:
>   362: 	debugfs_create_str("firmware_file", 0200, d, &firmware_file);

That too should be fixed, all should just create an "empty" string to
start with.

> test case:
> 1. create a NULL string node
> char *test_node = NULL;
> debugfs_create_str("test_node", 0600, parent_dir, &test_node);
> 
> 2. read the node, like bellow:
> cat /sys/kernel/debug/test_node

With your patch, you could change step 2 to do a write, and still cause
a crash :)

So let's fix this properly, let's just fail the creation of NULL
strings, and fix up all callers.

thanks,

greg k-h