Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 21:14:53 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     chris hyser <chris.hyser@oracle.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org,
        "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 1/4] debugfs: return EINVAL if write to unsigned simple
 files exceed storage.
Message-ID: <2023053058-favorably-snowbound-39eb@gregkh>
References: <20230530194012.44411-1-chris.hyser@oracle.com>
 <20230530194012.44411-2-chris.hyser@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230530194012.44411-2-chris.hyser@oracle.com>
Precedence: bulk

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@oracle.com>
> ---
>  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 wheel.

But really, why?  This is debugfs, if userspace messes something up like
this, why not just cast and be done with it?

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

thanks,

greg k-h