[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