Re: [PATCH 1/5] debugfs: Prevent NULL dereference reading from string property
From: Richard Fitzgerald
Date: Tue May 16 2023 - 14:04:59 EST
On 16/5/23 18:43, Greg KH wrote:
On Tue, May 16, 2023 at 06:29:52PM +0100, Richard Fitzgerald wrote:
On 16/5/23 17:33, Greg KH wrote:
On Tue, May 16, 2023 at 05:07:49PM +0100, Richard Fitzgerald wrote:
Check in debugfs_read_file_str() if the string pointer is NULL.
It is perfectly reasonable that a driver may wish to export a string
to debugfs that can have the value NULL to indicate empty/unused/ignore.
Does any in-kernel driver do this today?
I don't know. The history here is that I was using debugfs_create_str()
to add a debugfs to a driver and made these improvements along the way.
Ultimately I had a reason to use a custom reader implementation.
But as I'd already written these patches I thought I'd send them.
If not, why not fix up the driver instead?
Well... could do. Though it seems a bit odd to me that a driver
design should be forced by the debugfs API, instead of the debugfs API
fitting normal code design. It's pretty standard and idiomatic for code
to use if (!str) { /* bail */ } type logic, so why shouldn't the debugfs
API handle that?
Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
---
fs/debugfs/file.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..2c085ab4e800 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -878,6 +878,9 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
return ret;
str = *(char **)file->private_data;
+ if (!str)
+ return simple_read_from_buffer(user_buf, count, ppos, "\n", 1);
Why not print "(NULL)"?
Again, could do. My thought here is that a debugfs can be piped into
tools and having to insert a catch for "(NULL)" in the pipeline is a
nuisance. This is a bit different from a dmesg print, which is less
likely to be used this way or to guarantee machine-parsing.
However, I don't mind changing to "(NULL)" if you prefer.
If a driver wants an "empty" string, they should provide an empty
string. We don't do empty values for any other type of pointer, right?
Actually we really should just bail out with an error if this is NULL,
let's not paper over bad drivers like this.
I don't understand this comment.
I think you'll find there is a very large amount of kernel code that
uses a NULL value in a pointer to mean ignore/unspecified in
some way. This has always been accepted C coding style.
The whole idea that a driver is "bad" for signalling some state
by a pointer being NULL makes no sense.
Please ignore this patch chain. I really don't feel like writing
non-idiomatic C code just to work around badly designed debugfs APIs.
Better to write a custom read().
thanks,
greg k-h