Re: [PATCH 1/4] debugfs: return EINVAL if write to unsigned simple files exceed storage.

From: chris hyser
Date: Tue May 30 2023 - 17:20:40 EST

On 5/30/23 16:14, Greg KH wrote:
On Tue, May 30, 2023 at 03:40:09PM -0400, chris hyser wrote:
Writes to the debug files created by "debugfs_create_*" (u8/u16/u32/u64),
(x8/x16/x32/x64) should not silently succeed if the value exceeds the
storage space for the type and upper written bits are lost. Absent an
error, a read should return the last written value. Current behaviour is
to down cast the storage type thus losing upper bits (thus u64/x64
files are unaffected).

This patch ensures the written value fits into the specified storage space
returning EINVAL on error.

Signed-off-by: Chris Hyser <chris.hyser@xxxxxxxxxx>
Documentation/filesystems/debugfs.rst | 7 ++++---
fs/debugfs/file.c | 6 ++++++
2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
index dc35da8b8792..6f1ac8d7f108 100644
--- a/Documentation/filesystems/debugfs.rst
+++ b/Documentation/filesystems/debugfs.rst
@@ -85,9 +85,10 @@ created with any of::
struct dentry *parent, u64 *value);
These files support both reading and writing the given value; if a specific
-file should not be written to, simply set the mode bits accordingly. The
-values in these files are in decimal; if hexadecimal is more appropriate,
-the following functions can be used instead::
+file should not be written to, simply set the mode bits accordingly. Written
+values that exceed the storage for the type return EINVAL. The values in these
+files are in decimal; if hexadecimal is more appropriate, the following
+functions can be used instead::
void debugfs_create_x8(const char *name, umode_t mode,
struct dentry *parent, u8 *value);
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..743ddd04f8d8 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -429,6 +429,8 @@ static struct dentry *debugfs_create_mode_unsafe(const char *name, umode_t mode,
static int debugfs_u8_set(void *data, u64 val)
+ if (val > (1 << sizeof(u8) * 8) - 1)
+ return -EINVAL;

We do have U8_MAX and friends, please don't reinvent the whee >
But really, why? This is debugfs, if userspace messes something up like
this, why not just cast and be done with it?

So PeterZ asked much the same question when I submitted something for the divide by zero bug mentioned. Just don't write 0. Ultimately, he recommended looking into the more generic min/max stuff.

Now, if it was only "really" debug I think even then catching an easy to catch error is useful. Unfortunately, many distros ship with this stuff turned on and tools access these things ... which is the answer to your next question, what might this break. I'm not sure something (likely accidentally) relying on silently dropping bits really works, but it would appear to break with this change.

In other words, what existing workflow is now going to break with this
patchset? :)

I would answer this with your point that this is debugfs. I remember when a bunch of the sched debug sysctls were originally moved to debugfs files. That broke tools etc, but the argument was that it is debugfs. You should not use it in production and the API is what it is. Not an exact analogy for sure, but that was my thinking.

Honestly, I wasn't really sure if this patch would fly, but simple checks and feedback even in debugging seemed useful to me.

Are you open to the min/max files patch? I can resend w/o basing on this patch.