linux-kernel - Re: [PATCH] staging: unisys: Properly test 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: <20220322085558.GN3293@kadam>
Date:   Tue, 22 Mar 2022 11:55:58 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     David Kershner <david.kershner@...sys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        sparmaintainer@...sys.com, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: unisys: Properly test debugfs_create_dir()
 return values

On Tue, Mar 22, 2022 at 11:49:28AM +0300, Dan Carpenter wrote:
> On Tue, Mar 22, 2022 at 09:38:58AM +0100, Fabio M. De Francesco wrote:
> > debugfs_create_dir() returns a pointers to a dentry objects. On failures
> > it returns errors. Currently the values returned to visornic_probe()
> > seem to be tested for being equal to NULL in case of failures.
> > 
> > Properly test with "if (IS_ERR())" and then assign the correct error 
> > value to the "err" variable.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > ---
> >  drivers/staging/unisys/visornic/visornic_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> > index 643432458105..58d03f3d3173 100644
> > --- a/drivers/staging/unisys/visornic/visornic_main.c
> > +++ b/drivers/staging/unisys/visornic/visornic_main.c
> > @@ -1922,11 +1922,11 @@ static int visornic_probe(struct visor_device *dev)
> >  	/* create debug/sysfs directories */
> >  	devdata->eth_debugfs_dir = debugfs_create_dir(netdev->name,
> >  						      visornic_debugfs_dir);
> > -	if (!devdata->eth_debugfs_dir) {
> > +	if (IS_ERR(devdata->eth_debugfs_dir)) {
> 
> Normally I would say to just delete the error handling.

In the normal case checking debugfs functions for errors is not
required.  The users all have checks built in.  The exception is when
the driver dereferences devdata->eth_debugfs_dir itself.  So when you
make these kinds of changes do a grep for eth_debugfs_dir to make sure
it's not dereferenced, but normally it is not.

But again, here, delete everything.

regards,
dan carpenter