[PATCH 0/5] debugfs: Fixes and improvements to debugfs_create_str()

[PATCH 0/5] debugfs: Fixes and improvements to debugfs_create_str()

[Richard Fitzgerald]

Fix NULL-deference bug, kerneldoc errors, minor coding style error.
Also add a wrapper to avoid having to cast a const pointer to non-const.

Richard Fitzgerald (5):
  debugfs: Prevent NULL dereference reading from string property
  debugfs: Remove kerneldoc that says debugfs_create_str() returns a
    value
  debugfs: Update debugfs_create_str() kerneldoc to warn about pointer
    race
  debugfs: Move debugfs_create_str() export to correct location
  debugfs: Add debugfs_create_const_str()

 fs/debugfs/file.c       | 21 +++++++++++----------
 include/linux/debugfs.h | 27 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

-- 
2.30.2

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

[Richard Fitzgerald]

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

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) {
-- 
2.30.2

[PATCH 2/5] debugfs: Remove kerneldoc that says debugfs_create_str() returns a value

[Richard Fitzgerald]

Remove the lines of kerneldoc that say debugfs_create_str() returns a
struct dentry *. The function does not return a value.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 2c085ab4e800..0c039a3d9a42 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -943,15 +943,6 @@ static const struct file_operations fops_str_wo = {
  * This function creates a file in debugfs with the given name that
  * contains the value of the variable @value.  If the @mode variable is so
  * set, it can be read from, and written to.
- *
- * This function will return a pointer to a dentry if it succeeds.  This
- * pointer must be passed to the debugfs_remove() function when the file is
- * to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.)  If an error occurs, ERR_PTR(-ERROR) will be
- * returned.
- *
- * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV) will
- * be returned.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)
-- 
2.30.2

[PATCH 3/5] debugfs: Update debugfs_create_str() kerneldoc to warn about pointer race

[Richard Fitzgerald]

Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
value must not change after the function returns, because of a race with
debugfs_read_file_str() accessing the pointer.

The only safe case is a change from NULL to non-NULL because in that case
debugfs_read_file_str() will see either the NULL or the valid pointer.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 0c039a3d9a42..77794871f26d 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
  *          directory dentry if set.  If this parameter is %NULL, then the
  *          file will be created in the root of the debugfs filesystem.
  * @value: a pointer to the variable that the file should read to and write
- *         from.
+ *         from. The char* pointer must not change, except from NULL to
+ *         non-NULL.
  *
  * This function creates a file in debugfs with the given name that
  * contains the value of the variable @value.  If the @mode variable is so
  * set, it can be read from, and written to.
+ *
+ * The char* pointed to by @value must not change after calling this
+ * function EXCEPT that it may change from NULL to non-NULL. This is to
+ * prevent the file read from accessing a stale pointer. A change from
+ * NULL to non-NULL is the only safe change, because the read will
+ * instantaneously see either NULL or the valid pointer.
  */
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value)
-- 
2.30.2

[PATCH 4/5] debugfs: Move debugfs_create_str() export to correct location

[Richard Fitzgerald]

Move the EXPORT_SYMBOL_GPL for debugfs_create_str() to where it should be,
immediately after the function it refers to.

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

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 77794871f26d..d3892d01a8b4 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -902,7 +902,6 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(debugfs_create_str);
 
 static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
 				      size_t count, loff_t *ppos)
@@ -957,6 +956,7 @@ void debugfs_create_str(const char *name, umode_t mode,
 	debugfs_create_mode_unsafe(name, mode, parent, value, &fops_str,
 				   &fops_str_ro, &fops_str_wo);
 }
+EXPORT_SYMBOL_GPL(debugfs_create_str);
 
 static ssize_t read_file_blob(struct file *file, char __user *user_buf,
 			      size_t count, loff_t *ppos)
-- 
2.30.2

[PATCH 5/5] debugfs: Add debugfs_create_const_str()

[Richard Fitzgerald]

Add a wrapper for debugfs_create_str() that takes a const char **.

It's never nice to have to cast a const pointer to a non-const to be
able to pass it to an API. It always looks suspicious and it is relying
on "knowing" that it's safe. A function that explicitly takes a const
pointer is creating a contract that a const pointer is safe.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/linux/debugfs.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index ea2d919fd9c7..2723690aedd1 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -401,4 +401,31 @@ static inline void debugfs_create_xul(const char *name, umode_t mode,
 		debugfs_create_x64(name, mode, parent, (u64 *)value);
 }
 
+/**
+ * debugfs_create_const_str - create a debugfs file that is used to read a string value
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is %NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @value: a pointer to the variable that the file should read from.
+ *         The const char* pointer must not change, except from NULL to
+ *         non-NULL.
+ *
+ * This function creates a file in debugfs with the given name that
+ * contains the value of the variable @value.
+ *
+ * The const char* pointed to by @value must not change after calling this
+ * function EXCEPT that it may change from NULL to non-NULL. This is to
+ * prevent the file read from accessing a stale pointer. A change from
+ * NULL to non-NULL is the only safe change, because the read will
+ * instantaneously see either NULL or the valid pointer.
+ */
+static inline void debugfs_create_const_str(const char *name, umode_t mode,
+					    struct dentry *parent,
+					    const char **value)
+{
+	debugfs_create_str(name, mode & ~0222, parent, (char **)value);
+}
+
 #endif
-- 
2.30.2

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

[Greg KH]

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

Re: [PATCH 3/5] debugfs: Update debugfs_create_str() kerneldoc to warn about pointer race

[Greg KH]

On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
> Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
> value must not change after the function returns, because of a race with
> debugfs_read_file_str() accessing the pointer.
> 
> The only safe case is a change from NULL to non-NULL because in that case
> debugfs_read_file_str() will see either the NULL or the valid pointer.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  fs/debugfs/file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 0c039a3d9a42..77794871f26d 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
>   *          directory dentry if set.  If this parameter is %NULL, then the
>   *          file will be created in the root of the debugfs filesystem.
>   * @value: a pointer to the variable that the file should read to and write
> - *         from.
> + *         from. The char* pointer must not change, except from NULL to
> + *         non-NULL.

This feels odd.  Why wouldn't you want to change the string value?  Or
why would you?

And why is this one-way transition ok?

Given that this is only used internally, why is it exported?

thanks,

greg k-h

Re: [PATCH 5/5] debugfs: Add debugfs_create_const_str()

[Greg KH]

On Tue, May 16, 2023 at 05:07:53PM +0100, Richard Fitzgerald wrote:
> Add a wrapper for debugfs_create_str() that takes a const char **.
> 
> It's never nice to have to cast a const pointer to a non-const to be
> able to pass it to an API. It always looks suspicious and it is relying
> on "knowing" that it's safe. A function that explicitly takes a const
> pointer is creating a contract that a const pointer is safe.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  include/linux/debugfs.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index ea2d919fd9c7..2723690aedd1 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -401,4 +401,31 @@ static inline void debugfs_create_xul(const char *name, umode_t mode,
>  		debugfs_create_x64(name, mode, parent, (u64 *)value);
>  }
>  
> +/**
> + * debugfs_create_const_str - create a debugfs file that is used to read a string value
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have
> + * @parent: a pointer to the parent dentry for this file.  This should be a
> + *          directory dentry if set.  If this parameter is %NULL, then the
> + *          file will be created in the root of the debugfs filesystem.
> + * @value: a pointer to the variable that the file should read from.
> + *         The const char* pointer must not change, except from NULL to
> + *         non-NULL.
> + *
> + * This function creates a file in debugfs with the given name that
> + * contains the value of the variable @value.
> + *
> + * The const char* pointed to by @value must not change after calling this
> + * function EXCEPT that it may change from NULL to non-NULL. This is to
> + * prevent the file read from accessing a stale pointer. A change from
> + * NULL to non-NULL is the only safe change, because the read will
> + * instantaneously see either NULL or the valid pointer.
> + */
> +static inline void debugfs_create_const_str(const char *name, umode_t mode,
> +					    struct dentry *parent,
> +					    const char **value)
> +{
> +	debugfs_create_str(name, mode & ~0222, parent, (char **)value);

You just "know" it's safe to do this?  There is nothing in
debugfs_create_str() that would prevent future changes from violating
the "const" here, which makes this very unsafe to maintain over time.

This feels backwards, why not make debugfs_create_str() take the const
pointer instead?

thanks,

greg k-h

Re: [PATCH 5/5] debugfs: Add debugfs_create_const_str()

[Greg KH]

On Tue, May 16, 2023 at 05:07:53PM +0100, Richard Fitzgerald wrote:
> Add a wrapper for debugfs_create_str() that takes a const char **.
> 
> It's never nice to have to cast a const pointer to a non-const to be
> able to pass it to an API. It always looks suspicious and it is relying
> on "knowing" that it's safe. A function that explicitly takes a const
> pointer is creating a contract that a const pointer is safe.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  include/linux/debugfs.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index ea2d919fd9c7..2723690aedd1 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -401,4 +401,31 @@ static inline void debugfs_create_xul(const char *name, umode_t mode,
>  		debugfs_create_x64(name, mode, parent, (u64 *)value);
>  }
>  
> +/**
> + * debugfs_create_const_str - create a debugfs file that is used to read a string value
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have
> + * @parent: a pointer to the parent dentry for this file.  This should be a
> + *          directory dentry if set.  If this parameter is %NULL, then the
> + *          file will be created in the root of the debugfs filesystem.
> + * @value: a pointer to the variable that the file should read from.
> + *         The const char* pointer must not change, except from NULL to
> + *         non-NULL.
> + *
> + * This function creates a file in debugfs with the given name that
> + * contains the value of the variable @value.
> + *
> + * The const char* pointed to by @value must not change after calling this
> + * function EXCEPT that it may change from NULL to non-NULL. This is to
> + * prevent the file read from accessing a stale pointer. A change from
> + * NULL to non-NULL is the only safe change, because the read will
> + * instantaneously see either NULL or the valid pointer.
> + */
> +static inline void debugfs_create_const_str(const char *name, umode_t mode,
> +					    struct dentry *parent,
> +					    const char **value)
> +{
> +	debugfs_create_str(name, mode & ~0222, parent, (char **)value);
> +}

Also, we need a user of the new function in order to be able to add it,
otherwise I'll just delete it eventually :)

thanks,

greg k-h

Re: [PATCH 5/5] debugfs: Add debugfs_create_const_str()

[Greg KH]

On Tue, May 16, 2023 at 05:07:53PM +0100, Richard Fitzgerald wrote:
> Add a wrapper for debugfs_create_str() that takes a const char **.
> 
> It's never nice to have to cast a const pointer to a non-const to be
> able to pass it to an API. It always looks suspicious and it is relying
> on "knowing" that it's safe. A function that explicitly takes a const
> pointer is creating a contract that a const pointer is safe.
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  include/linux/debugfs.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index ea2d919fd9c7..2723690aedd1 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -401,4 +401,31 @@ static inline void debugfs_create_xul(const char *name, umode_t mode,
>  		debugfs_create_x64(name, mode, parent, (u64 *)value);
>  }
>  
> +/**
> + * debugfs_create_const_str - create a debugfs file that is used to read a string value
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have
> + * @parent: a pointer to the parent dentry for this file.  This should be a
> + *          directory dentry if set.  If this parameter is %NULL, then the
> + *          file will be created in the root of the debugfs filesystem.
> + * @value: a pointer to the variable that the file should read from.
> + *         The const char* pointer must not change, except from NULL to
> + *         non-NULL.
> + *
> + * This function creates a file in debugfs with the given name that
> + * contains the value of the variable @value.
> + *
> + * The const char* pointed to by @value must not change after calling this
> + * function EXCEPT that it may change from NULL to non-NULL. This is to
> + * prevent the file read from accessing a stale pointer. A change from
> + * NULL to non-NULL is the only safe change, because the read will
> + * instantaneously see either NULL or the valid pointer.
> + */
> +static inline void debugfs_create_const_str(const char *name, umode_t mode,
> +					    struct dentry *parent,
> +					    const char **value)
> +{
> +	debugfs_create_str(name, mode & ~0222, parent, (char **)value);
> +}

And you didn't include a version for when CONFIG_DEBUG_FS is not
enabled, which would cause anyone who used this function, to break the
build :(

thanks,

greg k-h

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

[Richard Fitzgerald]

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.

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

[Richard Fitzgerald]

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

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

[Greg KH]

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

Re: [PATCH 3/5] debugfs: Update debugfs_create_str() kerneldoc to warn about pointer race

[Richard Fitzgerald]

On 16/5/23 17:35, Greg KH wrote:
> On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
>> Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
>> value must not change after the function returns, because of a race with
>> debugfs_read_file_str() accessing the pointer.
>>
>> The only safe case is a change from NULL to non-NULL because in that case
>> debugfs_read_file_str() will see either the NULL or the valid pointer.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   fs/debugfs/file.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 0c039a3d9a42..77794871f26d 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
>>    *          directory dentry if set.  If this parameter is %NULL, then the
>>    *          file will be created in the root of the debugfs filesystem.
>>    * @value: a pointer to the variable that the file should read to and write
>> - *         from.
>> + *         from. The char* pointer must not change, except from NULL to
>> + *         non-NULL.
> 
> This feels odd.  Why wouldn't you want to change the string value?  Or
> why would you?

Well, if you _would_ want to change the string value, then the
implementation of debugfs_create_str() is certainly broken and could
only be fixed by involving a shared mutex to protect use of the pointer.

> 
> And why is this one-way transition ok?
> 

This one case happens to be safe because it either sees NULL (which it
handles) or a valid pointer (which is ok). It will not result in using a
stale pointer. This wasn't a deliberate design intent but happens to be
safe, and easily maintainable behavior.

A transition from valid->NULL or old->new isn't safe because the
read function could get the old pointer but racing with that is the
change to the pointer, and so the debugfs code could try to use a
stale pointer.

> Given that this is only used internally, why is it exported?
> 

It isn't only used internally. I found 3 drivers that use it.
But there are no uses internal to debugfs.

I didn't write debugfs_create_str(), I only tried to use it and made
an attempt to fix some problems.

Given the limitations of the basic implementation of
debugfs_create_str() and its file reading function (the lack of
protection against the pointer changing) perhaps drop this chain? Don't
bother fixing it, instead deprecate it for being unsafe?

> thanks,
> 
> greg k-h

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

[Richard Fitzgerald]

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

Re: [PATCH 3/5] debugfs: Update debugfs_create_str() kerneldoc to warn about pointer race

[Greg KH]

On Tue, May 16, 2023 at 06:50:16PM +0100, Richard Fitzgerald wrote:
> On 16/5/23 17:35, Greg KH wrote:
> > On Tue, May 16, 2023 at 05:07:51PM +0100, Richard Fitzgerald wrote:
> > > Add a warning to the debugfs_create_str() kerneldoc that the char * pointer
> > > value must not change after the function returns, because of a race with
> > > debugfs_read_file_str() accessing the pointer.
> > > 
> > > The only safe case is a change from NULL to non-NULL because in that case
> > > debugfs_read_file_str() will see either the NULL or the valid pointer.
> > > 
> > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> > > ---
> > >   fs/debugfs/file.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index 0c039a3d9a42..77794871f26d 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -938,11 +938,18 @@ static const struct file_operations fops_str_wo = {
> > >    *          directory dentry if set.  If this parameter is %NULL, then the
> > >    *          file will be created in the root of the debugfs filesystem.
> > >    * @value: a pointer to the variable that the file should read to and write
> > > - *         from.
> > > + *         from. The char* pointer must not change, except from NULL to
> > > + *         non-NULL.
> > 
> > This feels odd.  Why wouldn't you want to change the string value?  Or
> > why would you?
> 
> Well, if you _would_ want to change the string value, then the
> implementation of debugfs_create_str() is certainly broken and could
> only be fixed by involving a shared mutex to protect use of the pointer.

Agreed.  So let's just say "never change the pointer" and leave it at
that?

> > And why is this one-way transition ok?
> > 
> 
> This one case happens to be safe because it either sees NULL (which it
> handles) or a valid pointer (which is ok). It will not result in using a
> stale pointer. This wasn't a deliberate design intent but happens to be
> safe, and easily maintainable behavior.
> 
> A transition from valid->NULL or old->new isn't safe because the
> read function could get the old pointer but racing with that is the
> change to the pointer, and so the debugfs code could try to use a
> stale pointer.
> 
> > Given that this is only used internally, why is it exported?
> > 
> 
> It isn't only used internally. I found 3 drivers that use it.
> But there are no uses internal to debugfs.

Oops, I missed the other users (arm_scmi and opp), so let's leave it.

> I didn't write debugfs_create_str(), I only tried to use it and made
> an attempt to fix some problems.
> 
> Given the limitations of the basic implementation of
> debugfs_create_str() and its file reading function (the lack of
> protection against the pointer changing) perhaps drop this chain? Don't
> bother fixing it, instead deprecate it for being unsafe?

We don't "deprecate" things, that never works.  We either fix them, or
rip them out :)

thanks,

greg k-h

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

[Greg KH]

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