[Openib-windows] RE: User verbs
Leonid Keller
leonid at mellanox.co.il
Tue Mar 7 07:57:14 PST 2006
Looks OK.
BTW, why don't you do the change for ci_call in the consistent way ?
#define verbs_ci_call(h_ca, handle_array, num_handles, p_ci_op,
p_umv_buf ) \
h_ca->obj.p_ci_ca->verbs.vendor_call( \
(p_umv_buf) ? h_ca->h_um_ca :
h_ca->obj.p_ci_ca->h_ci_ca, \
handle_array, num_handles, p_ci_op, p_umv_buf )
> -----Original Message-----
> From: ftillier.sst at gmail.com [mailto:ftillier.sst at gmail.com]
> On Behalf Of Fabian Tillier
> Sent: Tuesday, March 07, 2006 1:17 AM
> To: Leonid Keller
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] RE: User verbs
>
> 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;
>
More information about the ofw
mailing list