Re: [PATCH] sysfs: clamp show() return value in sysfs_kf_read()

[David Laight <david.laight.linux@gmail.com>]

On Wed, 20 May 2026 15:07:01 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> sysfs_kf_seq_show() defends against buggy show() callbacks that return
> larger than PAGE_SIZE by clamping the value and printing a warning.
> sysfs_kf_read(), the prealloc variant, has no such defense.
> 
> The only current in-tree user of __ATTR_PREALLOC is drivers/md/md.c,
> whose show() callbacks are well-behaved, so this is hardening against
> future drivers doing foolish things and out-of-tree code doing even more
> foolish things.

What is the rational for it using PREALLOC?
From the code it looks like it could set a buffer size, but it doesn't seem to.
Which means there is a kmalloc(PAGE_SIZE + 1) in there.
If it did set a size, the check below would be wrong.

From the look of the fragment below it is just passed the address of the
pre-allocated buffer.
That also means it can't use sysfs_emit() because that relies on a
page-aligned buffer.

Also I suspect that kmalloc() grabbing a buffer from the per-cpu freelist
is cheaper than the mutex required to lock access to the preallocted buffer.

It would be 'nice' to change the type of the buffer passed to the show()
functions and to sysfs_emit() to something other than 'char *'.
Even if the initial change is just a typdef for 'char *' so that
you can tell which functions can call sysfs_emit().
At the moment it is pretty hard to actually decide whether it is safe
to change the printf() to sysfs_emit().

If all the show() functions are expected to include a terminating '\n'
then the wrapper code could add one if missing.
That would save a lot of strcat(buf, '\n'); return strlen(buf); sequences.

I also think (bad for me at 11pm) that the buffer should be 4k not PAGE_SIZE.

-- David


> 
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: NeilBrown <neil@brown.name>
> Cc: Tejun Heo <tj@kernel.org>
> Fixes: 2b75869bba67 ("sysfs/kernfs: allow attributes to request write buffer be pre-allocated.")
> Assisted-by: gregkh_clanker_t1000
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/sysfs/file.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 5709cede1d75..25b44fe171a3 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -120,6 +120,10 @@ static ssize_t sysfs_kf_read(struct kernfs_open_file *of, char *buf,
>  	len = ops->show(kobj, of->kn->priv, buf);
>  	if (len < 0)
>  		return len;
> +	if (len >= (ssize_t)PAGE_SIZE) {
> +		printk("fill_read_buffer: %pS returned bad count\n", ops->show);
> +		len = PAGE_SIZE - 1;
> +	}
>  	if (pos) {
>  		if (len <= pos)
>  			return 0;