| Message ID | alpine.LNX.2.00.1101172116330.27021@swampdragon.chaosbits.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Jan 17, 2011 at 09:24:57PM +0100, Jesper Juhl wrote: > In drivers/net/ns83820.c::ns83820_init_one() we dynamically allocate > memory via alloc_etherdev(). We then call PRIV() on the returned storage > which is 'return netdev_priv()'. netdev_priv() takes the pointer it is > passed and adds 'ALIGN(sizeof(struct net_device), NETDEV_ALIGN)' to it and > returns it. Then we test the resulting pointer for NULL, which it is > unlikely to be at this point, and later dereference it. This will go bad > if alloc_etherdev() actually returned NULL. > > This patch reworks the code slightly so that we test for a NULL pointer > (and return -ENOMEM) directly after calling alloc_etherdev(). > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> Yeah, the previous code was a little bit ugly. Good catch! > --- > ns83820.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > Compile tested only. I have no way to test this for real. > > diff --git a/drivers/net/ns83820.c b/drivers/net/ns83820.c > index 84134c7..a41b2cf 100644 > --- a/drivers/net/ns83820.c > +++ b/drivers/net/ns83820.c > @@ -1988,12 +1988,11 @@ static int __devinit ns83820_init_one(struct pci_dev *pci_dev, > } > > ndev = alloc_etherdev(sizeof(struct ns83820)); > - dev = PRIV(ndev); > - > err = -ENOMEM; > - if (!dev) > + if (!ndev) > goto out; > > + dev = PRIV(ndev); > dev->ndev = ndev; > > spin_lock_init(&dev->rx_info.lock); > > > -- > Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ > Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html > Plain text mails only, please. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 17, 2011 at 09:24:57PM +0100, Jesper Juhl wrote: > In drivers/net/ns83820.c::ns83820_init_one() we dynamically allocate > memory via alloc_etherdev(). We then call PRIV() on the returned storage > which is 'return netdev_priv()'. netdev_priv() takes the pointer it is > passed and adds 'ALIGN(sizeof(struct net_device), NETDEV_ALIGN)' to it and > returns it. Then we test the resulting pointer for NULL, which it is > unlikely to be at this point, and later dereference it. This will go bad > if alloc_etherdev() actually returned NULL. > > This patch reworks the code slightly so that we test for a NULL pointer > (and return -ENOMEM) directly after calling alloc_etherdev(). Looks good. -ben Signed-off-by: Benjamin LaHaise <bcrl@kvack.org> > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > ns83820.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > Compile tested only. I have no way to test this for real. > > diff --git a/drivers/net/ns83820.c b/drivers/net/ns83820.c > index 84134c7..a41b2cf 100644 > --- a/drivers/net/ns83820.c > +++ b/drivers/net/ns83820.c > @@ -1988,12 +1988,11 @@ static int __devinit ns83820_init_one(struct pci_dev *pci_dev, > } > > ndev = alloc_etherdev(sizeof(struct ns83820)); > - dev = PRIV(ndev); > - > err = -ENOMEM; > - if (!dev) > + if (!ndev) > goto out; > > + dev = PRIV(ndev); > dev->ndev = ndev; > > spin_lock_init(&dev->rx_info.lock); > > > -- > Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ > Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html > Plain text mails only, please. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Benjamin LaHaise <bcrl@kvack.org> Date: Tue, 18 Jan 2011 11:42:00 -0500 > On Mon, Jan 17, 2011 at 09:24:57PM +0100, Jesper Juhl wrote: >> In drivers/net/ns83820.c::ns83820_init_one() we dynamically allocate >> memory via alloc_etherdev(). We then call PRIV() on the returned storage >> which is 'return netdev_priv()'. netdev_priv() takes the pointer it is >> passed and adds 'ALIGN(sizeof(struct net_device), NETDEV_ALIGN)' to it and >> returns it. Then we test the resulting pointer for NULL, which it is >> unlikely to be at this point, and later dereference it. This will go bad >> if alloc_etherdev() actually returned NULL. >> >> This patch reworks the code slightly so that we test for a NULL pointer >> (and return -ENOMEM) directly after calling alloc_etherdev(). > > Looks good. > > -ben > > Signed-off-by: Benjamin LaHaise <bcrl@kvack.org> Applied, thanks everyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ns83820.c b/drivers/net/ns83820.c index 84134c7..a41b2cf 100644 --- a/drivers/net/ns83820.c +++ b/drivers/net/ns83820.c @@ -1988,12 +1988,11 @@ static int __devinit ns83820_init_one(struct pci_dev *pci_dev, } ndev = alloc_etherdev(sizeof(struct ns83820)); - dev = PRIV(ndev); - err = -ENOMEM; - if (!dev) + if (!ndev) goto out; + dev = PRIV(ndev); dev->ndev = ndev; spin_lock_init(&dev->rx_info.lock);
In drivers/net/ns83820.c::ns83820_init_one() we dynamically allocate memory via alloc_etherdev(). We then call PRIV() on the returned storage which is 'return netdev_priv()'. netdev_priv() takes the pointer it is passed and adds 'ALIGN(sizeof(struct net_device), NETDEV_ALIGN)' to it and returns it. Then we test the resulting pointer for NULL, which it is unlikely to be at this point, and later dereference it. This will go bad if alloc_etherdev() actually returned NULL. This patch reworks the code slightly so that we test for a NULL pointer (and return -ENOMEM) directly after calling alloc_etherdev(). Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- ns83820.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) Compile tested only. I have no way to test this for real.