2004-04-16 21:21:07

by Dave Jones

Subject: baycom_par dereference before check.

--- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:18:53.000000000 +0100
+++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:19:33.000000000 +0100
@@ -272,9 +272,13 @@
static void par96_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
struct net_device *dev = (struct net_device *)dev_id;
- struct baycom_state *bc = netdev_priv(dev);
+ struct baycom_state *bc;

- if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
+ if (!dev)
+ return;
+
+ bc = netdev_priv(dev);
+ if (!bc || bc->hdrv.magic != HDLCDRV_MAGIC)
return;

baycom_int_freq(bc);

2004-04-16 21:25:50

by Al Viro

Subject: Re: baycom_par dereference before check.

On Fri, Apr 16, 2004 at 10:20:04PM +0100, Dave Jones wrote:
>
> --- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:18:53.000000000 +0100
> +++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:19:33.000000000 +0100
> @@ -272,9 +272,13 @@
> static void par96_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> {
> struct net_device *dev = (struct net_device *)dev_id;
> - struct baycom_state *bc = netdev_priv(dev);
> + struct baycom_state *bc;
>
> - if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
> + if (!dev)
> + return;
> +
> + bc = netdev_priv(dev);

That's OK - netdev_priv(p) just adds a constant offset to p; no problem
with doing that to NULL.

2004-04-16 21:30:19

by Dave Jones

Subject: Re: baycom_par dereference before check.

On Fri, Apr 16, 2004 at 10:25:41PM +0100, [email protected] wrote:

> > +++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:19:33.000000000 +0100
> > @@ -272,9 +272,13 @@
> > static void par96_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> > {
> > struct net_device *dev = (struct net_device *)dev_id;
> > - struct baycom_state *bc = netdev_priv(dev);
> > + struct baycom_state *bc;
> >
> > - if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
> > + if (!dev)
> > + return;
> > +
> > + bc = netdev_priv(dev);
>
> That's OK - netdev_priv(p) just adds a constant offset to p; no problem
> with doing that to NULL.

Good point. Still doesn't strike me as particularly nice though.

Dave

2004-04-16 21:52:46

by Jeff Garzik

Subject: Re: baycom_par dereference before check.

Dave Jones wrote:
> On Fri, Apr 16, 2004 at 10:25:41PM +0100, [email protected] wrote:
>
> > > +++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:19:33.000000000 +0100
> > > @@ -272,9 +272,13 @@
> > > static void par96_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> > > {
> > > struct net_device *dev = (struct net_device *)dev_id;
> > > - struct baycom_state *bc = netdev_priv(dev);
> > > + struct baycom_state *bc;
> > >
> > > - if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
> > > + if (!dev)
> > > + return;
> > > +
> > > + bc = netdev_priv(dev);
> >
> > That's OK - netdev_priv(p) just adds a constant offset to p; no problem
> > with doing that to NULL.
>
> Good point. Still doesn't strike me as particularly nice though.


I would rather just remove the checks completely. The success of any of
those checks is a BUG() condition that should never occur.

Jeff

2004-04-16 21:59:16

by Dave Jones

Subject: Re: baycom_par dereference before check.

On Fri, Apr 16, 2004 at 05:48:57PM -0400, Jeff Garzik wrote:

> >Good point. Still doesn't strike me as particularly nice though.
> I would rather just remove the checks completely. The success of any of
> those checks is a BUG() condition that should never occur.

That works for me too. How about this?

Dave

--- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:52:35.000000000 +0100
+++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:52:53.000000000 +0100
@@ -274,8 +274,8 @@
struct net_device *dev = (struct net_device *)dev_id;
struct baycom_state *bc = netdev_priv(dev);

- if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
- return;
+ BUG_ON(!bc);
+ BUG_ON(bc->hdrv.magic != HDLCDRV_MAGIC);

baycom_int_freq(bc);
/*

2004-04-16 23:08:04

by Chris Wright

Subject: Re: baycom_par dereference before check.

* Dave Jones ([email protected]) wrote:
> On Fri, Apr 16, 2004 at 05:48:57PM -0400, Jeff Garzik wrote:
> > >Good point. Still doesn't strike me as particularly nice though.
> > I would rather just remove the checks completely. The success of any of
> > those checks is a BUG() condition that should never occur.
>
> That works for me too. How about this?
>
> Dave
>
> --- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:52:35.000000000 +0100
> +++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:52:53.000000000 +0100
> @@ -274,8 +274,8 @@
> struct net_device *dev = (struct net_device *)dev_id;
> struct baycom_state *bc = netdev_priv(dev);
>
> - if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
> - return;
> + BUG_ON(!bc);

If it's adding a constant offset to dev, the bc would be non-NULL in a bug
case, no?

thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net

2004-04-16 23:43:53

by Jeff Garzik

Subject: Re: baycom_par dereference before check.

Chris Wright wrote:
> * Dave Jones ([email protected]) wrote:
>
>>On Fri, Apr 16, 2004 at 05:48:57PM -0400, Jeff Garzik wrote:
>> > >Good point. Still doesn't strike me as particularly nice though.
>> > I would rather just remove the checks completely. The success of any of
>> > those checks is a BUG() condition that should never occur.
>>
>>That works for me too. How about this?
>>
>> Dave
>>
>>--- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:52:35.000000000 +0100
>>+++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:52:53.000000000 +0100
>>@@ -274,8 +274,8 @@
>> struct net_device *dev = (struct net_device *)dev_id;
>> struct baycom_state *bc = netdev_priv(dev);
>>
>>- if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
>>- return;
>>+ BUG_ON(!bc);
>
>
> If it's adding a constant offset to dev, the bc would be non-NULL in a bug
> case, no?

I'm just going to remove the checks completely.

Jeff

2004-04-19 16:40:07

by Jeff Garzik

Subject: Re: baycom_par dereference before check.

Dave Jones wrote:
> On Fri, Apr 16, 2004 at 05:48:57PM -0400, Jeff Garzik wrote:
>
> > >Good point. Still doesn't strike me as particularly nice though.
> > I would rather just remove the checks completely. The success of any of
> > those checks is a BUG() condition that should never occur.
>
> That works for me too. How about this?
>
> Dave
>
> --- linux-2.6.5/drivers/net/hamradio/baycom_par.c~ 2004-04-16 22:52:35.000000000 +0100
> +++ linux-2.6.5/drivers/net/hamradio/baycom_par.c 2004-04-16 22:52:53.000000000 +0100
> @@ -274,8 +274,8 @@
> struct net_device *dev = (struct net_device *)dev_id;
> struct baycom_state *bc = netdev_priv(dev);
>
> - if (!dev || !bc || bc->hdrv.magic != HDLCDRV_MAGIC)
> - return;
> + BUG_ON(!bc);
> + BUG_ON(bc->hdrv.magic != HDLCDRV_MAGIC);


I applied a patch which simplied removed the checks.

Jeff