linux-kernel - Re: [v4l-dvb-maintainer] drivers/media/dvb/dvb-core/dvb

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] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.58.0706151621000.4914@shell4.speakeasy.net>
Date:	Fri, 15 Jun 2007 16:58:04 -0700 (PDT)
From:	Trent Piepho <xyzzy@...akeasy.org>
To:	Adrian Bunk <bunk@...sta.de>
cc:	Michal CIJOML Semler <cijoml@...ny.cz>,
	Markus Rechberger <markus.rechberger@....com>,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	v4l-dvb-maintainer@...uxtv.org, linux-kernel@...r.kernel.org
Subject: Re: [v4l-dvb-maintainer] drivers/media/dvb/dvb-core/dvb_net.c:
 check-after-use

On Fri, 15 Jun 2007, Adrian Bunk wrote:
> The Coverity checker spotted the following obvious check-after-use in
> drivers/media/dvb/dvb-core/dvb_net.c:

I spotted it before the patch was even applied:
http://www.linuxtv.org/pipermail/v4l-dvb-maintainer/2007-April/003917.html
If only coverity would get people to fix their bugs too.

I'm almost certain the check for !dvbdev ( = file->private_data) is not
necessary.

If you trace it, starting from the beginning:

dvb device is created:
dvbdev.c:425  cdev_init(&dvb_device_cdev, &dvb_device_fops);

dvb_device_fops defined:
dvbdev.c:117 static struct file_operations dvb_device_fops =
dvbdev.c:118 {
dvbdev.c:119         .owner =        THIS_MODULE,
dvbdev.c:120         .open =         dvb_device_open,
dvbdev.c:121 };

BTW, this struct should be const, no?  In fact, why not make it a static
local to "__init init_dvbdev()", the only place it's used?  That way it
will go into the init.data section, right?

Anyway, the relevant code for dvb_device_open:

dvbdev.c:87  static int dvb_device_open(struct inode *inode, struct file *file)
dvbdev.c:91          dvbdev = dvbdev_find_device (iminor(inode));
dvbdev.c:93          if (dvbdev && dvbdev->fops) {
dvbdev.c:101                 file->private_data = dvbdev;
dvbdev.c:111                 return err;
dvbdev.c:112         }
dvbdev.c:113         return -ENODEV;

So file->private_data must be non-NULL when the file is opened, or the open
function for the dvb device will return ENODEV.

The only code code in the dvb tree that assigns to private_data that I
could find is in the open function for dmx devices, and it doesn't apply to
dvb net devices and can't set private_data to NULL anyway.

So, if there file->private_data starts as non-NULL, and there is no code
that changes it, it can't ever be non-NULL, right?

BTW, I hate checks for a pointer being NULL if it's not actually part of
the design that the pointer can be NULL.  It just clutters the code, as
other programmers see the check and they need to handle the pointer being
NULL too.  And if there is a bug and the pointer is NULL, the check doesn't
fix the bug it just masks it.  The farther it is from a bug's cause to the
point it's detected, the harder it is to fix.

Here is a patch to fix it.  Actually, there is still a check-after-use
after the patch, but is Coverity smart enough to find it?

--------------------------------------------------------------------------
diff -r 2f04637aaea8 linux/drivers/media/dvb/dvb-core/dvb_net.c
--- a/linux/drivers/media/dvb/dvb-core/dvb_net.c	Fri Jun 08 08:58:41 2007 -0300
+++ b/linux/drivers/media/dvb/dvb-core/dvb_net.c	Fri Jun 15 15:34:08 2007 -0700
@@ -1502,18 +1502,9 @@ static int dvb_net_close(struct inode *i
 	struct dvb_device *dvbdev = file->private_data;
 	struct dvb_net *dvbnet = dvbdev->priv;

-	if (!dvbdev)
-		return -ENODEV;
-
-	if ((file->f_flags & O_ACCMODE) == O_RDONLY) {
-		dvbdev->readers++;
-	} else {
-		dvbdev->writers++;
-	}
-
-	dvbdev->users++;
-
-	if(dvbdev->users == 1 && dvbnet->exit==1) {
+	dvb_generic_release(inode, file);
+
+	if(dvbdev->users == 1 && dvbnet->exit == 1) {
 		fops_put(file->f_op);
 		file->f_op = NULL;
 		wake_up(&dvbdev->wait_queue);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/