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, 16 May 2023 19:43:49 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Richard Fitzgerald <rf@opensource.cirrus.com>
Cc:     rafael@kernel.org, linux-kernel@vger.kernel.org,
        patches@opensource.cirrus.com
Subject: Re: [PATCH 1/5] debugfs: Prevent NULL dereference reading from
 string property
Message-ID: <2023051642-tiling-manlike-7536@gregkh>
References: <20230516160753.32317-1-rf@opensource.cirrus.com>
 <20230516160753.32317-2-rf@opensource.cirrus.com>
 <2023051659-sinless-lemon-e3b1@gregkh>
 <705c4511-bfba-ea46-1aad-b3783c1b21ae@opensource.cirrus.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <705c4511-bfba-ea46-1aad-b3783c1b21ae@opensource.cirrus.com>
Precedence: bulk

On Tue, May 16, 2023 at 06:29:52PM +0100, Richard Fitzgerald wrote:
> On 16/5/23 17:33, Greg KH wrote:
> > On Tue, May 16, 2023 at 05:07:49PM +0100, Richard Fitzgerald wrote:
> > > Check in debugfs_read_file_str() if the string pointer is NULL.
> > > 
> > > It is perfectly reasonable that a driver may wish to export a string
> > > to debugfs that can have the value NULL to indicate empty/unused/ignore.
> > 
> > Does any in-kernel driver do this today?
> 
> I don't know. The history here is that I was using debugfs_create_str()
> to add a debugfs to a driver and made these improvements along the way.
> Ultimately I had a reason to use a custom reader implementation.
> But as I'd already written these patches I thought I'd send them.
> 
> > 
> > If not, why not fix up the driver instead?
> > 
> 
> Well... could do. Though it seems a bit odd to me that a driver
> design should be forced by the debugfs API, instead of the debugfs API
> fitting normal code design. It's pretty standard and idiomatic for code
> to use if (!str) { /* bail */ } type logic, so why shouldn't the debugfs
> API handle that?
> 
> > > 
> > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> > > ---
> > >   fs/debugfs/file.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > > index 1f971c880dde..2c085ab4e800 100644
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -878,6 +878,9 @@ ssize_t debugfs_read_file_str(struct file *file, char __user *user_buf,
> > >   		return ret;
> > >   	str = *(char **)file->private_data;
> > > +	if (!str)
> > > +		return simple_read_from_buffer(user_buf, count, ppos, "\n", 1);
> > 
> > Why not print "(NULL)"?
> > 
> 
> Again, could do. My thought here is that a debugfs can be piped into
> tools and having to insert a catch for "(NULL)" in the pipeline is a
> nuisance. This is a bit different from a dmesg print, which is less
> likely to be used this way or to guarantee machine-parsing.
> However, I don't mind changing to "(NULL)" if you prefer.

If a driver wants an "empty" string, they should provide an empty
string.  We don't do empty values for any other type of pointer, right?

Actually we really should just bail out with an error if this is NULL,
let's not paper over bad drivers like this.

thanks,

greg k-h