diff mbox series

[RESEND,v5,2/4] fs: debugfs: Add write functionality to debugfs blobs

Message ID 20231107213647.1405493-3-avadhut.naik@amd.com
State New
Headers
Series Add support for Vendor Defined Error Types in Einj Module |

Commit Message

Avadhut Naik Nov. 7, 2023, 9:36 p.m. UTC 
  From: Avadhut Naik <Avadhut.Naik@amd.com>

Currently, debugfs_create_blob() creates read-only debugfs binary blob
files.

In some cases, however, userspace tools need to write variable length
data structures into predetermined memory addresses. An example is when
injecting Vendor-defined error types through the einj module. In such
cases, the functionality to write to these blob files in debugfs would
be desired since the mapping aspect can be handled within the modules
with userspace tools only needing to write into the blob files.

Implement a write callback to enable writing to these blob files in
debugfs.

Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/debugfs/file.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

Comments

Luck, Tony Nov. 7, 2023, 10:28 p.m. UTC | #1 
> @@ -1042,7 +1060,7 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
> 				   struct dentry *parent,
> 				   struct debugfs_blob_wrapper *blob)
> {
> -	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
> +	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
> }

The minimalist change here would be to s/0444/0666/

That would just allow callers to ask for writeable files without letting them
add execute permission, or exotic modes like setuid etc.

-Tony
Naik, Avadhut Nov. 8, 2023, 6:09 p.m. UTC | #2 
Hi,

On 11/7/2023 16:28, Luck, Tony wrote:
>> @@ -1042,7 +1060,7 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>> 				   struct dentry *parent,
>> 				   struct debugfs_blob_wrapper *blob)
>> {
>> -	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
>> +	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
>> }
> 
> The minimalist change here would be to s/0444/0666/
> 
> That would just allow callers to ask for writeable files without letting them
> add execute permission, or exotic modes like setuid etc.
> 
Noted. Thanks for the clarification. Will change to something like below:

	return debugfs_create_file_unsafe(name, mode & 0666, parent, blob, &fops_blob);

> -Tony
Naik, Avadhut Nov. 16, 2023, 5:54 p.m. UTC | #3 
Hi Tony,

On 11/7/2023 16:28, Luck, Tony wrote:
>> @@ -1042,7 +1060,7 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>> 				   struct dentry *parent,
>> 				   struct debugfs_blob_wrapper *blob)
>> {
>> -	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
>> +	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
>> }
> 
> The minimalist change here would be to s/0444/0666/
> 
Just realized that s/0444/0644/ might be an even more minimalist change since you anyways,
I think, need to be root for error injection through einj. Does that sound good?

In any case, using 0666 will result in the below checkpatch warning:

[root avadnaik-linux]# ./scripts/checkpatch.pl --strict -g HEAD
WARNING: Exporting world writable files is usually an error. Consider more restrictive permissions.
#84: FILE: fs/debugfs/file.c:1063:
+       return debugfs_create_file_unsafe(name, mode & 0666, parent, blob, &fops_blob);

total: 0 errors, 1 warnings, 0 checks, 54 lines checked

Would you be okay with s/0444/0644/?

-       return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
+       return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);

> That would just allow callers to ask for writeable files without letting them
> add execute permission, or exotic modes like setuid etc.
> 
> -Tony
Luck, Tony Nov. 16, 2023, 6:44 p.m. UTC | #4 
> > The minimalist change here would be to s/0444/0666/
> >
> Just realized that s/0444/0644/ might be an even more minimalist change since you anyways,
> I think, need to be root for error injection through einj. Does that sound good?

You need write access. I don't think you need to be root. E.g. a validation system might
set up an "einj" group and "chmod" all these files to 0664. But that's nitpicking.

>
> In any case, using 0666 will result in the below checkpatch warning:
>
> [root avadnaik-linux]# ./scripts/checkpatch.pl --strict -g HEAD
> WARNING: Exporting world writable files is usually an error. Consider more restrictive permissions.
> #84: FILE: fs/debugfs/file.c:1063:
> +       return debugfs_create_file_unsafe(name, mode & 0666, parent, blob, &fops_blob);
>
> total: 0 errors, 1 warnings, 0 checks, 54 lines checked

The warning is dubious. This code isn't necessarily exporting a world writeable file. But
it does allow a caller of this routine to do that.

>
> Would you be okay with s/0444/0644/?

> -       return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
> +       return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);


Yes. This is fine (better). Make sure to mention in the commit comment that this allows
callers to create files writeable by owner.

-Tony
Naik, Avadhut Nov. 16, 2023, 9:46 p.m. UTC | #5 
On 11/16/2023 12:44, Luck, Tony wrote:
>>> The minimalist change here would be to s/0444/0666/
>>>
>> Just realized that s/0444/0644/ might be an even more minimalist change since you anyways,
>> I think, need to be root for error injection through einj. Does that sound good?
> 
> You need write access. I don't think you need to be root. E.g. a validation system might
> set up an "einj" group and "chmod" all these files to 0664. But that's nitpicking.
> 
>>
>> In any case, using 0666 will result in the below checkpatch warning:
>>
>> [root avadnaik-linux]# ./scripts/checkpatch.pl --strict -g HEAD
>> WARNING: Exporting world writable files is usually an error. Consider more restrictive permissions.
>> #84: FILE: fs/debugfs/file.c:1063:
>> +       return debugfs_create_file_unsafe(name, mode & 0666, parent, blob, &fops_blob);
>>
>> total: 0 errors, 1 warnings, 0 checks, 54 lines checked
> 
> The warning is dubious. This code isn't necessarily exporting a world writeable file. But
> it does allow a caller of this routine to do that.
> 
>>
>> Would you be okay with s/0444/0644/?
> 
>> -       return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
>> +       return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
> 
> 
> Yes. This is fine (better). Make sure to mention in the commit comment that this allows
> callers to create files writeable by owner.
> 
Will do. Thanks for the confirmation!
> -Tony
> 
>
diff mbox series

Patch

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index c45e8c2d62e1..dde1f5f30fb3 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1008,17 +1008,35 @@  static ssize_t read_file_blob(struct file *file, char __user *user_buf,
 	return r;
 }
 
+static ssize_t write_file_blob(struct file *file, const char __user *user_buf,
+			       size_t count, loff_t *ppos)
+{
+	struct debugfs_blob_wrapper *blob = file->private_data;
+	struct dentry *dentry = F_DENTRY(file);
+	ssize_t r;
+
+	r = debugfs_file_get(dentry);
+	if (unlikely(r))
+		return r;
+	r = simple_write_to_buffer(blob->data, blob->size, ppos, user_buf,
+				   count);
+
+	debugfs_file_put(dentry);
+	return r;
+}
+
 static const struct file_operations fops_blob = {
 	.read =		read_file_blob,
+	.write =	write_file_blob,
 	.open =		simple_open,
 	.llseek =	default_llseek,
 };
 
 /**
- * debugfs_create_blob - create a debugfs file that is used to read a binary blob
+ * debugfs_create_blob - create a debugfs file that is used to read and write
+ * a binary blob
  * @name: a pointer to a string containing the name of the file to create.
- * @mode: the read permission that the file should have (other permissions are
- *	  masked out)
+ * @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.
@@ -1027,7 +1045,7 @@  static const struct file_operations fops_blob = {
  *
  * This function creates a file in debugfs with the given name that exports
  * @blob->data as a binary blob. If the @mode variable is so set it can be
- * read from. Writing is not supported.
+ * 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
@@ -1042,7 +1060,7 @@  struct dentry *debugfs_create_blob(const char *name, umode_t mode,
 				   struct dentry *parent,
 				   struct debugfs_blob_wrapper *blob)
 {
-	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
+	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_blob);