[Openib-windows] RE: User verbs

Fabian Tillier ftillier at silverstorm.com
Mon Mar 6 15:17:23 PST 2006


On 3/2/06, Fabian Tillier <ftillier at silverstorm.com> wrote:
> Hi Leonid,
>
> On 3/2/06, Leonid Keller <leonid at mellanox.co.il> wrote:
>
> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c
> > ===================================================================
> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c   (revision 401)
> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c   (revision 1031)
> > @@ -582,6 +582,7 @@
> >        {
> >                /* Copy the dev info. */
> >                p_um_ca->dev_info = *hca_ul_info;
> > +               p_um_ca->hob_p = hob_p;
> >                *ph_um_ca = (ib_ca_handle_t)p_um_ca;
> >                (*(void** __ptr64)p_umv_buf->p_inout_buf) =
> > p_um_ca->p_mapped_addr;
> >                p_umv_buf->status = IB_SUCCESS;
> > @@ -644,7 +645,7 @@
> >                OUT                     ib_pd_handle_t
> > *ph_pd,
> >        IN      OUT                     ci_umv_buf_t
> > *p_umv_buf )
> >  {
> > -       mlnx_hob_t                              *hob_p = (mlnx_hob_t
> > *)h_ca;
> > +       mlnx_hob_t                              *hob_p;
> >        mlnx_hobul_t                    *hobul_p;
> >        HH_hca_dev_t                    *hca_ul_info;
> >        HHUL_pd_hndl_t                  hhul_pd_hndl = 0;
> > @@ -655,6 +656,11 @@
> >
> >        CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);
> >
> > +       if( p_umv_buf && p_umv_buf->command )
> > +               hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
> > +       else
> > +               hob_p = (mlnx_hob_t *)h_ca;
>
> This won't work properly - when the CA is opened by a user-mode
> process without the kernel bypass provider (either it is missing,
> corrupted, or whatever other failure), the p_umv_buf->command is zero
> to mlnx_um_ca, in which case it returns NULL for the mlnx_um_ca_t
> object.  This will cause a NULL dereference in the code above.
>
> There are a few ways of fixing this.  First, we can keep checking the
> command, and in mlnx_um_open return the mlnx_hob_t if command is zero.
>  This way h_um_ca is the same as h_ca.
>
> Another way is to always allocate the mlnx_um_ca_t object, even if
> p_umv_buf->command is zero, and remove the check for the command when
> resolving the HCA object.
>
> I'm leaning towards the first, since it's simplest.  I'll implement
> that and send out a patch for approval.

The first introduces a security vulnerabilty as it lets the kernel
driver select how to cast based on a user-mode input (the command). 
The only way to solve this is to test only on p_umv_buf, which is
allocated by the proxy in kernel-mode.  This required changing
um_open_ca to always allocate the mlnx_um_ca_t structure for user-mode
clients.

Here's a patch that implements this, as well as adding support for
ci_call.  This should open the door for the FW burning enhancements
you had planned without the security issues of the previous attempts. 
Let me know if it looks good and I will commit it.

Thanks,

- Fab

Index: core/al/kernel/al_ci_ca.c
===================================================================
--- core/al/kernel/al_ci_ca.c	(revision 225)
+++ core/al/kernel/al_ci_ca.c	(working copy)
@@ -31,6 +31,7 @@
  */

 #include "al_ci_ca.h"
+#include "al_verbs.h"
 #include "al_cq.h"
 #include "al_debug.h"
 #include "al_mad_pool.h"
@@ -486,9 +487,8 @@

 	if( h_ca->obj.p_ci_ca->verbs.vendor_call )
 	{
-		status = h_ca->obj.p_ci_ca->verbs.vendor_call(
-			h_ca->obj.p_ci_ca->h_ci_ca, p_handle_array, num_handles,
-			p_ci_op, p_umv_buf );
+		status = verbs_ci_call(
+			h_ca, p_handle_array, num_handles, p_ci_op, p_umv_buf );
 	}
 	else
 	{
Index: core/al/al_verbs.h
===================================================================
--- core/al/al_verbs.h	(revision 225)
+++ core/al/al_verbs.h	(working copy)
@@ -71,8 +71,9 @@
 		port_num, ca_mod, p_port_attr_mod )

 #define verbs_create_cq(h_ca, p_cq_create, h_cq) \
-	h_ca->obj.p_ci_ca->verbs.create_cq( h_ca->obj.p_ci_ca->h_ci_ca,\
-	h_cq, &p_cq_create->size, &h_cq->h_ci_cq, p_umv_buf )
+	h_ca->obj.p_ci_ca->verbs.create_cq( \
+		(p_umv_buf) ? h_ca->h_um_ca : h_ca->obj.p_ci_ca->h_ci_ca, \
+		h_cq, &p_cq_create->size, &h_cq->h_ci_cq, p_umv_buf )

 #define verbs_check_cq(h_cq)	((h_cq)->h_ci_cq)
 #define verbs_destroy_cq(h_cq) \
@@ -169,8 +170,9 @@
 		h_qp->h_ci_qp, p_mw_bind, p_rkey )

 #define verbs_allocate_pd(h_ca, h_pd) \
-	h_ca->obj.p_ci_ca->verbs.allocate_pd(\
-				h_ca->obj.p_ci_ca->h_ci_ca, h_pd->type, &h_pd->h_ci_pd, p_umv_buf )
+	h_ca->obj.p_ci_ca->verbs.allocate_pd( \
+		(p_umv_buf) ? h_ca->h_um_ca : h_ca->obj.p_ci_ca->h_ci_ca, \
+		h_pd->type, &h_pd->h_ci_pd, p_umv_buf )

 /*
  * Reference the hardware PD.
@@ -304,6 +306,26 @@
 		h_mcast->obj.p_ci_ca->verbs.detach_mcast( \
 			h_mcast->h_ci_mcast )

+static inline ib_api_status_t
+verbs_ci_call(
+	IN				ib_ca_handle_t				h_ca,
+	IN		const	void* __ptr64 *		const	handle_array	OPTIONAL,
+	IN				uint32_t					num_handles,
+	IN				ib_ci_op_t*			const	p_ci_op,
+	IN				ci_umv_buf_t*		const	p_umv_buf OPTIONAL )
+{
+	if( p_umv_buf )
+	{
+		return h_ca->obj.p_ci_ca->verbs.vendor_call(
+			h_ca->h_um_ca, handle_array, num_handles, p_ci_op, p_umv_buf );
+	}
+	else
+	{
+		return h_ca->obj.p_ci_ca->verbs.vendor_call(
+			h_ca->obj.p_ci_ca->h_ci_ca, handle_array,
+			num_handles, p_ci_op, p_umv_buf );
+	}
+}


 #else
Index: hw/mt23108/kernel/hca_driver.c
===================================================================
--- hw/mt23108/kernel/hca_driver.c	(revision 225)
+++ hw/mt23108/kernel/hca_driver.c	(working copy)
@@ -1310,7 +1310,10 @@
 	UNREFERENCED_PARAMETER(p_umv_buf);

 	status =  STATUS_SUCCESS;
-	p_hob = (mlnx_hob_t *)(const void *)p_context;
+	if( p_umv_buf )
+		p_hob = ((mlnx_um_ca_t* __ptr64)p_context)->hob_p;
+	else
+		p_hob = (mlnx_hob_t *)(const void *)p_context;

 	p_dev_obj = (DEVICE_OBJECT *)(const void *)p_hob->p_dev_obj;
 	p_ci =  p_ci_op;
Index: hw/mt23108/kernel/hca_data.h
===================================================================
--- hw/mt23108/kernel/hca_data.h	(revision 225)
+++ hw/mt23108/kernel/hca_data.h	(working copy)
@@ -210,6 +210,8 @@
 	MDL					*p_mdl;
 	void				*p_mapped_addr;
 	HH_hca_hndl_t		hh_hndl;
+	mlnx_hob_t			*hob_p;
+	/* The next two fields must be grouped together as the are mapped to UM. */
 	HH_hca_dev_t		dev_info;
 	uint8_t				ul_hca_res[1];	// Beginning of UL resource buffer.

Index: hw/mt23108/kernel/hca_verbs.c
===================================================================
--- hw/mt23108/kernel/hca_verbs.c	(revision 225)
+++ hw/mt23108/kernel/hca_verbs.c	(working copy)
@@ -520,8 +520,20 @@

 	if( !p_umv_buf->command )
 	{
+		p_um_ca = (mlnx_um_ca_t*)cl_zalloc( sizeof(mlnx_um_ca_t) );
+		if( !p_um_ca )
+		{
+			p_umv_buf->status = IB_INSUFFICIENT_MEMORY;
+			goto mlnx_um_open_err1;
+		}
+		/* Copy the dev info. */
+		p_um_ca->dev_info = *hca_ul_info;
+		p_um_ca->hob_p = hob_p;
+		*ph_um_ca = (ib_ca_handle_t)p_um_ca;
 		p_umv_buf->status = IB_SUCCESS;
-		goto mlnx_um_open_err1;
+		p_umv_buf->output_size = 0;
+		HCA_EXIT( MLNX_DBG_TRACE );
+		return IB_SUCCESS;
 	}

 	/*
@@ -582,6 +594,7 @@
 	{
 		/* Copy the dev info. */
 		p_um_ca->dev_info = *hca_ul_info;
+		p_um_ca->hob_p = hob_p;
 		*ph_um_ca = (ib_ca_handle_t)p_um_ca;
 		(*(void** __ptr64)p_umv_buf->p_inout_buf) = p_um_ca->p_mapped_addr;
 		p_umv_buf->status = IB_SUCCESS;
@@ -622,11 +635,15 @@
 	if( !p_um_ca )
 		return;

+	if( !p_um_ca->p_mapped_addr )
+		goto done;
+
 	THH_hob_free_ul_res( hh_hndl, p_um_ca->ul_hca_res );

 mlnx_um_close_cleanup:
 	MmUnmapLockedPages( p_um_ca->p_mapped_addr, p_um_ca->p_mdl );
 	IoFreeMdl( p_um_ca->p_mdl );
+done:
 	cl_free( p_um_ca );

 	HCA_EXIT( MLNX_DBG_TRACE );
@@ -644,7 +661,7 @@
 		OUT			ib_pd_handle_t				*ph_pd,
 	IN	OUT			ci_umv_buf_t				*p_umv_buf )
 {
-	mlnx_hob_t				*hob_p = (mlnx_hob_t *)h_ca;
+	mlnx_hob_t				*hob_p;
 	mlnx_hobul_t			*hobul_p;
 	HH_hca_dev_t			*hca_ul_info;
 	HHUL_pd_hndl_t			hhul_pd_hndl = 0;
@@ -655,6 +672,11 @@

 	CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);

+	if( p_umv_buf )
+		hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
+	else
+		hob_p = (mlnx_hob_t *)h_ca;
+	
 	hobul_p = mlnx_hobs_get_hobul(hob_p);
 	if (NULL == hobul_p) {
 		status = IB_INVALID_CA_HANDLE;
@@ -1889,7 +1911,7 @@
 {
 	ib_api_status_t			status;

-	mlnx_hob_t				*hob_p = (mlnx_hob_t *)h_ca;
+	mlnx_hob_t				*hob_p;
 	u_int32_t				cq_idx;
 	u_int32_t				cq_num;
 	u_int32_t				cq_size = 0;
@@ -1901,6 +1923,11 @@

 	CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);

+	if( p_umv_buf )
+		hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
+	else
+		hob_p = (mlnx_hob_t *)h_ca;
+
 	hobul_p = mlnx_hobs_get_hobul(hob_p);
 	if (NULL == hobul_p) {
 		status = IB_INVALID_CA_HANDLE;
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: user_verbs2.patch
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20060306/d9740de6/attachment.ksh>


More information about the ofw mailing list