linux-kernel - Re: [PATCH v3 1/3] debugfs: Add write support to debugfs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2023081203-happier-mutable-e4f0@gregkh>
Date:   Sat, 12 Aug 2023 12:40:00 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Mike Tipton <quic_mdtipton@...cinc.com>
Cc:     djakov@...nel.org, rafael@...nel.org, corbet@....net,
        linux-pm@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        quic_okukatla@...cinc.com, quic_viveka@...cinc.com,
        peterz@...radead.org, quic_pkondeti@...cinc.com
Subject: Re: [PATCH v3 1/3] debugfs: Add write support to debugfs_create_str()

On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote:
> Currently, debugfs_create_str() only supports reading strings from
> debugfs. Add support for writing them as well.
> 
> Based on original implementation by Peter Zijlstra [0]. Write support
> was present in the initial patch version, but dropped in v2 due to lack
> of users. We have a user now, so reintroduce it.
> 
> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/
> 
> Signed-off-by: Mike Tipton <quic_mdtipton@...cinc.com>
> ---
>  fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index b7711888dd17..87b3753aa4b1 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
>  static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
>  				      size_t count, loff_t *ppos)
>  {
> -	/* This is really only for read-only strings */
> -	return -EINVAL;
> +	struct dentry *dentry = F_DENTRY(file);
> +	char *old, *new = NULL;
> +	int pos = *ppos;
> +	int r;
> +
> +	r = debugfs_file_get(dentry);
> +	if (unlikely(r))
> +		return r;
> +
> +	old = *(char **)file->private_data;
> +
> +	/* only allow strict concatenation */
> +	r = -EINVAL;
> +	if (pos && pos != strlen(old))
> +		goto error;
> +
> +	r = -E2BIG;
> +	if (pos + count + 1 > PAGE_SIZE)
> +		goto error;
> +
> +	r = -ENOMEM;
> +	new = kmalloc(pos + count + 1, GFP_KERNEL);
> +	if (!new)
> +		goto error;
> +
> +	if (pos)
> +		memcpy(new, old, pos);
> +
> +	r = -EFAULT;
> +	if (copy_from_user(new + pos, user_buf, count))
> +		goto error;
> +
> +	new[pos + count] = '\0';
> +	strim(new);
> +
> +	rcu_assign_pointer(*(char **)file->private_data, new);
> +	synchronize_rcu();
> +	kfree(old);
> +
> +	debugfs_file_put(dentry);
> +	return count;
> +
> +error:
> +	kfree(new);
> +	debugfs_file_put(dentry);
> +	return r;
>  }

So you just added write support for ALL debugfs files that use the
string interface, what did you just allow to break?

I recommend just using your own debugfs file function instead, as this
could cause bad problems, right?  Are you sure that all string calls can
handle the variable be freed underneath it like this call will allow to
happen?

So I wouldn't recommend doing this, sorry.

greg k-h