Re: [PATCH] sb1000: prevent a potential NULL pointer dereference in sb1000_dev_ioctl()

On 29/07/07 00:02 +0200, Jesper Juhl wrote:

Hi,

Here's a small patch, prompted by a find by the Coverity checker, 
that removes a potential NULL pointer dereference from 
drivers/net/sb1000.c::sb1000_dev_ioctl().
The checker spotted that we do a NULL test of 'dev', yet we 
dereference the pointer prior to that check.
This patch simply moves the dereference after the NULL test.

But... it can't be called without a valid 'dev', no?
A quick 'grep do_ioctl net/' confirms that all calls are in
the form of 'dev->do_ioctl(dev, ...'.


	Domen

	

On Sun, 29 Jul 2007, Domen Puncer wrote:

On 29/07/07 00:02 +0200, Jesper Juhl wrote:

quoted

Hi,

Here's a small patch, prompted by a find by the Coverity checker, 
that removes a potential NULL pointer dereference from 
drivers/net/sb1000.c::sb1000_dev_ioctl().
The checker spotted that we do a NULL test of 'dev', yet we 
dereference the pointer prior to that check.
This patch simply moves the dereference after the NULL test.

But... it can't be called without a valid 'dev', no?
A quick 'grep do_ioctl net/' confirms that all calls are in
the form of 'dev->do_ioctl(dev, ...'.

Yup, I think so too ...

quoted

@@ -991,11 +991,13 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	short PID[4];
 	int ioaddr[2], status, frequency;
 	unsigned int stats[5];
-	struct sb1000_private *lp = netdev_priv(dev);
+	struct sb1000_private *lp;
 
 	if (!(dev && dev->flags & IFF_UP))
 		return -ENODEV;

I think we could get rid of the !dev check itself. Actually, the IFF_UP
check /also/ looks suspect to me for two reasons: (1) I remember Stephen
Hemminger once telling me dev->flags is legacy and unsafe, and one of
the netif_xxx() functions be used instead, and, (2) I wonder if we really
require the interface to be up and *running* when we do this ioctl.


Satyam

On Sun, 29 Jul 2007, Domen Puncer wrote:

quoted

On 29/07/07 00:02 +0200, Jesper Juhl wrote:

quoted

Hi,

Here's a small patch, prompted by a find by the Coverity checker, 
that removes a potential NULL pointer dereference from 
drivers/net/sb1000.c::sb1000_dev_ioctl().
The checker spotted that we do a NULL test of 'dev', yet we 
dereference the pointer prior to that check.
This patch simply moves the dereference after the NULL test.

But... it can't be called without a valid 'dev', no?
A quick 'grep do_ioctl net/' confirms that all calls are in
the form of 'dev->do_ioctl(dev, ...'.

Yup, I think so too ...

quoted

quoted

@@ -991,11 +991,13 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	short PID[4];
 	int ioaddr[2], status, frequency;
 	unsigned int stats[5];
-	struct sb1000_private *lp = netdev_priv(dev);
+	struct sb1000_private *lp;
 
 	if (!(dev && dev->flags & IFF_UP))
 		return -ENODEV;

I think we could get rid of the !dev check itself. Actually, the IFF_UP
check /also/ looks suspect to me for two reasons: (1) I remember Stephen
Hemminger once telling me dev->flags is legacy and unsafe, and one of
the netif_xxx() functions be used instead, and, (2) I wonder if we really
require the interface to be up and *running* when we do this ioctl.

Updated patch below.

[PATCH] sb1000: Remove bogus checks

In net_device->do_ioctl() of the sb1000 driver (sb1000_dev_ioctl):

(1) !dev condition is always false -- this function cannot be called with
NULL net_device.
(2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
can (and should) be allowed even when the interface is not up and running.

So let's remove these checks.

Signed-off-by: Satyam Sharma <redacted>
Cc: Jesper Juhl <redacted>
Cc: Domen Puncer <redacted>

---

 drivers/net/sb1000.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/sb1000.c b/drivers/net/sb1000.c
index 1de3eec..f60fe98 100644
--- a/drivers/net/sb1000.c
+++ b/drivers/net/sb1000.c

@@ -993,9 +993,6 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	unsigned int stats[5];
 	struct sb1000_private *lp = netdev_priv(dev);
 
-	if (!(dev && dev->flags & IFF_UP))
-		return -ENODEV;
-
 	ioaddr[0] = dev->base_addr;
 	/* mem_start holds the second I/O address */
 	ioaddr[1] = dev->mem_start;

On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:

(2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
can (and should) be allowed even when the interface is not up and running.

Are you _sure_? This function does poke with the device hardware.
It might return crap or even machinecheck when not initialized.
Hardware is probably powered down, if not IFF_UP. (I don't know if that's
the case here, though).

-- 
Greetings Michael.

Hi Michael,


On Sun, 29 Jul 2007, Michael Buesch wrote:

On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:

quoted

(2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
can (and should) be allowed even when the interface is not up and running.

Are you _sure_? This function does poke with the device hardware.
It might return crap or even machinecheck when not initialized.
Hardware is probably powered down, if not IFF_UP. (I don't know if that's
the case here, though).

IFF_UP checks if the _interface_ is up -- the hardware / card could still
be powered up, but the interface down (ifconfing eth0 down or ip link set
eth0 down).

Probably what we want here is netif_device_present()? -- I think that
should return true only when the *device* itself is up (as in powered)
but the interface itself could be down ...

Let's wait for comments from the netdev people Cc:'ed here, in that case.

quoted

 drivers/net/sb1000.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/sb1000.c b/drivers/net/sb1000.c
index 1de3eec..f60fe98 100644
--- a/drivers/net/sb1000.c
+++ b/drivers/net/sb1000.c

@@ -993,9 +993,6 @@ static int sb1000_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	unsigned int stats[5];
 	struct sb1000_private *lp = netdev_priv(dev);
 
-	if (!(dev && dev->flags & IFF_UP))
-		return -ENODEV;
-

Satyam

On Sunday 29 July 2007 21:09, Satyam Sharma wrote:

Hi Michael,


On Sun, 29 Jul 2007, Michael Buesch wrote:

quoted

On Sunday 29 July 2007 20:34:46 Satyam Sharma wrote:

quoted

(2) !(dev->flags & IFF_UP) is bogus because the functions of this ioctl
can (and should) be allowed even when the interface is not up and running.

Are you _sure_? This function does poke with the device hardware.
It might return crap or even machinecheck when not initialized.
Hardware is probably powered down, if not IFF_UP. (I don't know if that's
the case here, though).

IFF_UP checks if the _interface_ is up -- the hardware / card could still
be powered up, but the interface down (ifconfing eth0 down or ip link set
eth0 down).

Well, that is device/driver dependent and I don't know what's
the case for this driver. It's encouraged to shutdown hardware
completely (except the WOL parts) when the interface is down.
Dunno if this driver does it. But _if_ it does it, it could cause
problems to poke with the hardware while down.