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

[chris hyser]

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.

-chrish