diff mbox series

[1/5] debugfs: Prevent NULL dereference reading from string property

Message ID 20230516160753.32317-2-rf@opensource.cirrus.com
State New
Headers
Series debugfs: Fixes and improvements to debugfs_create_str() |

Commit Message

Richard Fitzgerald May 16, 2023, 4:07 p.m. UTC 
  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.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 fs/debugfs/file.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Greg KH May 16, 2023, 4:33 p.m. UTC | #1 
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?

If not, why not fix up the driver instead?

> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  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)"?

thanks,

greg k-h
Richard Fitzgerald May 16, 2023, 5:14 p.m. UTC | #2 
On 16/5/23 17:07, Richard Fitzgerald wrote:
> --- 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);
> +

Oh, this isn't right. I've somehow sent an older version that is missing
the call to debugfs_file_put(). Sorry. I'll send a v2 chain.
Richard Fitzgerald May 16, 2023, 5:29 p.m. UTC | #3 
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@opensource.cirrus.com>
>> ---
>>   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.

> thanks,
> 
> greg k-h
Greg KH May 16, 2023, 5:43 p.m. UTC | #4 
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@opensource.cirrus.com>
> > > ---
> > >   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.

thanks,

greg k-h
Richard Fitzgerald May 16, 2023, 6:04 p.m. UTC | #5 
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@opensource.cirrus.com>
>>>> ---
>>>>    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
Greg KH May 17, 2023, 6:19 a.m. UTC | #6 
On Tue, May 16, 2023 at 07:04:42PM +0100, Richard Fitzgerald wrote:
> 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@opensource.cirrus.com>
> > > > > ---
> > > > >    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.

The whole idea of passing a NULL pointer to debugfs makes no sense :)

If a driver does this, then they deserve the crash, let's just say "do
not do that" and leave it at that please.

> 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().

Let's fix the badly designed debugfs apis please, it's not good to have
code that is impossible to use correctly.

thanks,

greg k-h
diff mbox series

Patch

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);
+
 	len = strlen(str) + 1;
 	copy = kmalloc(len, GFP_KERNEL);
 	if (!copy) {