Patchwork vxge: potential NULL dereference

login
register
mail settings
Submitter Dan Carpenter
Date Sept. 10, 2010, 11:54 a.m.
Message ID <20100910115423.GC5959@bicker>
Download mbox | patch
Permalink /patch/64382/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Dan Carpenter - Sept. 10, 2010, 11:54 a.m.
At the start of the function we test whether the "vpath" is NULL but we
need another test here as well.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
This is a static checker bug, I'm not sure if we ever pass a NULL
pointer for "vpath".

--
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
David Miller - Sept. 10, 2010, 8:32 p.m.
From: Dan Carpenter <error27@gmail.com>
Date: Fri, 10 Sep 2010 13:54:23 +0200

> At the start of the function we test whether the "vpath" is NULL but we
> need another test here as well.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> This is a static checker bug, I'm not sure if we ever pass a NULL
> pointer for "vpath".

I cannot see any case where this can happen.  There are two
cases:

1) __vxge_hw_vpath_alarm_process() is invoked via vxge_hw_device_begin_irq(),
   which looks like:

			ret = __vxge_hw_vpath_alarm_process(
				&hldev->virtual_paths[i], skip_alarms);

   that vpath pointer first argument will never be NULL.

2) __vxge_hw_vpath_alarm_process() is invoked via vxge_hw_vpath_alarm_process()
   which uses:

	status = __vxge_hw_vpath_alarm_process(vp->vpath, skip_alarms);

   All vpath valid active vpath handles always have a non-NULL vp->vpath
   virtual path back pointer, as setup by vxge_hw_vpath_open():

 ...
	vp->vpath = vpath;
...
	*vpath_handle = vp;

	attr->fifo_attr.userdata = vpath->fifoh;
	attr->ring_attr.userdata = vpath->ringh;

	return VXGE_HW_OK;

So we can simply remove the first NULL check as this can never actually
be NULL.
--
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
Dan Carpenter - Sept. 10, 2010, 9:12 p.m.
On Fri, Sep 10, 2010 at 01:32:55PM -0700, David Miller wrote:
> So we can simply remove the first NULL check as this can never actually
> be NULL.

Ok.  Thanks, Dave.  I should have looked more into this myself.  I'll
send a patch to remove the check tomorrow.

regards,
dan carpenter

--
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
Jon Mason - Sept. 10, 2010, 9:32 p.m.
On Fri, Sep 10, 2010 at 01:32:55PM -0700, David Miller wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Fri, 10 Sep 2010 13:54:23 +0200
> 
> > At the start of the function we test whether the "vpath" is NULL but we
> > need another test here as well.
> > 
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > ---
> > This is a static checker bug, I'm not sure if we ever pass a NULL
> > pointer for "vpath".
> 
> I cannot see any case where this can happen.  There are two
> cases:
> 
> 1) __vxge_hw_vpath_alarm_process() is invoked via vxge_hw_device_begin_irq(),
>    which looks like:
> 
> 			ret = __vxge_hw_vpath_alarm_process(
> 				&hldev->virtual_paths[i], skip_alarms);
> 
>    that vpath pointer first argument will never be NULL.

It is possible to the vpath to be NULL in this array if it is not
populated in __vxge_hw_vp_initialize due to the vpath being masked off
my the adapter.  vxge_hw_device_begin_irq calls
__vxge_hw_vpath_alarm_process on all possible vpaths regardless of
their allocation.  This is the case we need to worry about.

It is not an issue because in the first instance of the vpath being
NULL, its sets the alarm_event to be VXGE_HW_EVENT_UNKNOWN.  The first
check in the out2 error path checks for VXGE_HW_EVENT_UNKNOWN and
returns.  So its not possible to hit this...though it is ugly code.  I
welcome a reworking of the code to something mroe elegant. :)

Thanks,
Jon

> 
> 2) __vxge_hw_vpath_alarm_process() is invoked via vxge_hw_vpath_alarm_process()
>    which uses:
> 
> 	status = __vxge_hw_vpath_alarm_process(vp->vpath, skip_alarms);
> 
>    All vpath valid active vpath handles always have a non-NULL vp->vpath
>    virtual path back pointer, as setup by vxge_hw_vpath_open():
> 
>  ...
> 	vp->vpath = vpath;
> ...
> 	*vpath_handle = vp;
> 
> 	attr->fifo_attr.userdata = vpath->fifoh;
> 	attr->ring_attr.userdata = vpath->ringh;
> 
> 	return VXGE_HW_OK;
> 
> So we can simply remove the first NULL check as this can never actually
> be NULL.
--
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
David Miller - Sept. 10, 2010, 9:40 p.m.
From: Jon Mason <jon.mason@exar.com>
Date: Fri, 10 Sep 2010 17:32:15 -0400

> It is possible to the vpath to be NULL in this array if it is not
> populated in __vxge_hw_vp_initialize due to the vpath being masked off
> my the adapter.  vxge_hw_device_begin_irq calls
> __vxge_hw_vpath_alarm_process on all possible vpaths regardless of
> their allocation.  This is the case we need to worry about.
> 
> It is not an issue because in the first instance of the vpath being
> NULL, its sets the alarm_event to be VXGE_HW_EVENT_UNKNOWN.  The first
> check in the out2 error path checks for VXGE_HW_EVENT_UNKNOWN and
> returns.  So its not possible to hit this...though it is ugly code.  I
> welcome a reworking of the code to something mroe elegant. :)

Aha, I see, thanks for explaining this.
--
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

Patch

diff --git a/drivers/net/vxge/vxge-traffic.c b/drivers/net/vxge/vxge-traffic.c
index cedf08f..1790748 100644
--- a/drivers/net/vxge/vxge-traffic.c
+++ b/drivers/net/vxge/vxge-traffic.c
@@ -2157,7 +2157,8 @@  out2:
 		(alarm_event == VXGE_HW_EVENT_UNKNOWN))
 		return VXGE_HW_OK;
 
-	__vxge_hw_device_handle_error(hldev, vpath->vp_id, alarm_event);
+	if (vpath)
+		__vxge_hw_device_handle_error(hldev, vpath->vp_id, alarm_event);
 
 	if (alarm_event == VXGE_HW_EVENT_SERR)
 		return VXGE_HW_ERR_CRITICAL;