NET: netpoll, fix potential NULL ptr dereference
in [Kernel]
|
From: Laurent Chavey on 16 Mar 2010 13:00 Acked-by: chavey(a)google.com On Tue, Mar 16, 2010 at 8:29 AM, Jiri Slaby <jslaby(a)suse.cz> wrote: > Stanse found that one error path in netpoll_setup dereferences npinfo > even though it is NULL. Avoid that by adding new label and go to that > instead. > > Signed-off-by: Jiri Slaby <jslaby(a)suse.cz> > Cc: Daniel Borkmann <danborkmann(a)googlemail.com> > Cc: David S. Miller <davem(a)davemloft.net> > --- > �net/core/netpoll.c | � �4 ++-- > �1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 7aa6972..d4ec38f 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np) > � � � � � � � �npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > � � � � � � � �if (!npinfo) { > � � � � � � � � � � � �err = -ENOMEM; > - � � � � � � � � � � � goto release; > + � � � � � � � � � � � goto put; > � � � � � � � �} > > � � � � � � � �npinfo->rx_flags = 0; > @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np) > > � � � � � � � �kfree(npinfo); > � � � �} > - > +put: > � � � �dev_put(ndev); > � � � �return err; > �} > -- > 1.7.0.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at �http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matt Mackall on 16 Mar 2010 13:20 On Tue, 2010-03-16 at 16:29 +0100, Jiri Slaby wrote: > Stanse found that one error path in netpoll_setup dereferences npinfo > even though it is NULL. Avoid that by adding new label and go to that > instead. > > Signed-off-by: Jiri Slaby <jslaby(a)suse.cz> > Cc: Daniel Borkmann <danborkmann(a)googlemail.com> > Cc: David S. Miller <davem(a)davemloft.net> > --- > net/core/netpoll.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 7aa6972..d4ec38f 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np) > npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > if (!npinfo) { > err = -ENOMEM; > - goto release; > + goto put; > } > > npinfo->rx_flags = 0; > @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np) > > kfree(npinfo); > } > - > +put: > dev_put(ndev); > return err; > } I don't get it. The source of the branch tests for !ndev->npinfo and the original destination of the branch also tests for !ndev->npinfo. I don't see how it gets dereferenced. This looks like it just patches over a false positive in your tool (which isn't correlating the validity of npinfo with ndev->npinfo) without actually improving the code. However, it seems that we can drop the second check at release if we add your new exit point. -- http://selenic.com : development and support for Mercurial and Linux -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Slaby on 16 Mar 2010 13:30 On 03/16/2010 06:12 PM, Matt Mackall wrote: > I don't get it. The source of the branch tests for !ndev->npinfo and the > original destination of the branch also tests for !ndev->npinfo. I don't > see how it gets dereferenced. Let's look at more of the context: if (!ndev->npinfo) { npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); if (!npinfo) { // npinfo is NULL err = -ENOMEM; goto release; } .... release: // npinfo is still NULL if (!ndev->npinfo) { // condition is the same (holds) // dereference below: vvvvvvvvvvvvvvv spin_lock_irqsave(&npinfo->rx_lock, flags); list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { npe->dev = NULL; } spin_unlock_irqrestore(&npinfo->rx_lock, flags); kfree(npinfo); } thanks, -- js -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matt Mackall on 16 Mar 2010 14:00 On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote: > On 03/16/2010 06:12 PM, Matt Mackall wrote: > > I don't get it. The source of the branch tests for !ndev->npinfo and the > > original destination of the branch also tests for !ndev->npinfo. I don't > > see how it gets dereferenced. > > Let's look at more of the context: > if (!ndev->npinfo) { > npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > if (!npinfo) { // npinfo is NULL > err = -ENOMEM; > goto release; > } > ... > release: // npinfo is still NULL > if (!ndev->npinfo) { // condition is the same (holds) > // dereference below: vvvvvvvvvvvvvvv > spin_lock_irqsave(&npinfo->rx_lock, flags); > list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { > npe->dev = NULL; > } > spin_unlock_irqrestore(&npinfo->rx_lock, flags); > > kfree(npinfo); > } Ok, you're correct, I read the second test backwards. Acked-by: Matt Mackall <mpm(a)selenic.com> -- http://selenic.com : development and support for Mercurial and Linux -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Miller on 16 Mar 2010 17:30 From: Matt Mackall <mpm(a)selenic.com> Date: Tue, 16 Mar 2010 12:56:00 -0500 > On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote: >> On 03/16/2010 06:12 PM, Matt Mackall wrote: >> > I don't get it. The source of the branch tests for !ndev->npinfo and the >> > original destination of the branch also tests for !ndev->npinfo. I don't >> > see how it gets dereferenced. >> >> Let's look at more of the context: >> if (!ndev->npinfo) { >> npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); >> if (!npinfo) { // npinfo is NULL >> err = -ENOMEM; >> goto release; >> } >> ... >> release: // npinfo is still NULL >> if (!ndev->npinfo) { // condition is the same (holds) >> // dereference below: vvvvvvvvvvvvvvv >> spin_lock_irqsave(&npinfo->rx_lock, flags); >> list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { >> npe->dev = NULL; >> } >> spin_unlock_irqrestore(&npinfo->rx_lock, flags); >> >> kfree(npinfo); >> } > > Ok, you're correct, I read the second test backwards. > > Acked-by: Matt Mackall <mpm(a)selenic.com> Applied, thanks everyone. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: PCMCIA: resource, fix lock imbalance Next: SCSI: lpfc, fix lock imbalances |