[Openib-windows] [PATCH] 2 new vendor calls
Fabian Tillier
ftillier at silverstorm.com
Wed Sep 21 11:59:35 PDT 2005
On 9/21/05, Leonid Keller <leonid at mellanox.co.il> wrote:
>
>
> i've added 2 new vendor calls for "tools" (like FW burning tool flint.exe):
> FW_MAP_CRSPACE - maps HCA's CR SPACE into user's space and returns
> VA and an opaque object for unmap.
> FW_UNMAP_CRSPACE - unmaps CR SPACE and releases resources.
>
> Any comments are appreciated.
>
>
>
>
> Signed-off-by: Leonid Keller(leonid at mellanox.co.il)
> Index:
> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c
> ===================================================================
> ---
> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c
> (revision 438)
> +++
> S:/svn.wininf/trunk/hw/mt23108/vapi/mlxsys/os_dep/win/tdriver/MdPnp.c
> (revision 439)
> @@ -490,11 +490,18 @@
> {
> if(
> l_pIrpStack->Parameters.QueryInterface.InterfaceSpecificData
> )
> {
> - // Our interface. Return the HH HCA handle
> in
> - // the "InterfaceSpecificData" member.
> -
> *(HH_hca_hndl_t*)l_pIrpStack->Parameters.QueryInterface.
> - InterfaceSpecificData =
> l_pMdDevContext->m_hHhHca;
> - pi_pIrp->IoStatus.Status = STATUS_SUCCESS;
> + struct _hca_if {
> + HH_hca_hndl_t hh_hndl;
> + void * kernel_crspace_addr;
> + ULONG kernel_crspace_size;
> + } *if_p =
> l_pIrpStack->Parameters.QueryInterface.InterfaceSpecificData;
> +
> + // Our interface. Return the HH HCA handle
> and other data
> + if_p->hh_hndl = l_pMdDevContext->m_hHhHca;
> + if_p->kernel_crspace_addr =
> l_pMdDevContext->m_Cr.m_pKernelAddr;
> + if_p->kernel_crspace_size =
> l_pMdDevContext->m_Cr.m_ulKernelSize;
> +
> + pi_pIrp->IoStatus.Status = STATUS_SUCCESS;
> l_Status = STATUS_SUCCESS;
> break;
> }
> Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c
> ===================================================================
> --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c
> (revision 438)
> +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.c
> (revision 439)
> @@ -142,6 +142,28 @@
> return p_hca;
> }
>
> +mlnx_hca_t*
> +mlnx_hca_from_hh_hndl(
> + IN HH_hca_hndl_t
> hh_hndl )
> +{
> + cl_list_item_t *p_item;
> + mlnx_hca_t *p_hca = NULL;
> +
> + cl_spinlock_acquire( &hob_lock );
> + p_item = cl_qlist_head( &mlnx_hca_list );
> + while( p_item != cl_qlist_end( &mlnx_hca_list ) )
> + {
> + p_hca = PARENT_STRUCT( p_item, mlnx_hca_t, list_item );
> + if( p_hca->hh_hndl == hh_hndl )
> + break;
> + p_item = cl_qlist_next( p_item );
> + p_hca = NULL;
> + }
> + cl_spinlock_release( &hob_lock );
> + return p_hca;
> +}
> +
> +
> /*
> void
> mlnx_names_from_guid(
> Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c
> ===================================================================
> --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c
> (revision 438)
> +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.c
> (revision 439)
> @@ -540,8 +540,11 @@
> p_io_stack->Parameters.QueryInterface.Version = 1;
You must increment the version number if you change the interface definition.
> p_io_stack->Parameters.QueryInterface.Size = 0;
> p_io_stack->Parameters.QueryInterface.Interface =
> NULL;
> -
> p_io_stack->Parameters.QueryInterface.InterfaceSpecificData
> =
> - &p_ext->hca.hh_hndl;
> + {
> + void *p = &p_ext->hca.s;
> + memset( p, 0, sizeof(p_ext->hca.s) );
Why not just do memset( &p_ext->hca.s, 0, sizeof(p_ext->hca.s) )?
> +
> p_io_stack->Parameters.QueryInterface.InterfaceSpecificData
> = p;
> + }
Why not just use &p_ext->hca.s rather than the local variable p? This
would make the code more readable.
> p_io_stack->Parameters.QueryInterface.InterfaceType
> =
> &GUID_MD_INTERFACE;
> p_irp->IoStatus.Status = STATUS_NOT_SUPPORTED;
> @@ -562,7 +565,8 @@
> ("Query interface for HCA handle returned %08x.\n",
> status) );
> return status;
> }
> -
> + p_ext->hca.hh_hndl = p_ext->hca.s.hh_hndl;
> +
Why duplicate the handle. Just change use of p_ext->hca.hh_hndl to
p_ext->hca.s.hh_hndl and save yourself the trouble.
> HCA_EXIT( HCA_DBG_PNP );
> return status;
> }
> @@ -1296,6 +1300,107 @@
> return status;
> }
>
> +static NTSTATUS
> +__map_crspace(
> + IN mlnx_hob_t *
> p_hob,
> + IN PVOID
> p_buf,
> + IN ULONG
> buf_size
> + )
> +{
> + NTSTATUS status;
> + PMDL mdl_p;
> + mlnx_hca_t *p_hca =
> mlnx_hca_from_hh_hndl(p_hob->hh_hndl);
Rather than this, please change the definition of mlnx_hob_t to point
to the mlnx_hca_t. This should be trivial, and can replace the copy
of the device object pointer at hca_verbs.c at 114
When you change mlnx_hob_t, please remove the copy of the device
object pointer as well as the copy of the hh_hndl, since both of these
can be accessed from the mlnx_hca_t.
Once you do that, you can eliminate this lookup function all together.
> + PVOID ua, ka;
> + ULONG sz;
> + struct _map_crspace *res_p = (struct _map_crspace *)p_buf;
> +
> + HCA_ENTER( HCA_DBG_PNP );
> +
> + // sanity checks
> + if ( buf_size < sizeof *res_p ) {
> + status = STATUS_INVALID_PARAMETER;
> + goto out;
> + }
> + if (p_hca == NULL) {
> + status = STATUS_UNSUCCESSFUL;
> + goto out;
> + }
> + ka = p_hca->s.kernel_crspace_addr;
> + sz = p_hca->s.kernel_crspace_size;
> + if ( sz == 0 || ka == NULL) {
> + HCA_TRACE( HCA_DBG_ERROR, ("No kernel mapping of CR
> space.\n") );
> + status = STATUS_UNSUCCESSFUL;
> + goto out;
> + }
> +
> + // prepare for mapping to user space
> + mdl_p = IoAllocateMdl( ka, sz, FALSE,FALSE,NULL);
> + if (mdl_p == NULL) {
> + HCA_TRACE( HCA_DBG_ERROR, ("IoAllocateMdl failed.\n") );
> + status = STATUS_INSUFFICIENT_RESOURCES;
> + goto out;
> + }
> +
> + // fill MDL
> + MmBuildMdlForNonPagedPool(mdl_p);
> +
> + // map the buffer into user space
> + ua = MmMapLockedPagesSpecifyCache( mdl_p, UserMode, MmNonCached,
> + NULL, FALSE, NormalPagePriority );
This must be done withing a try/except block. See the docs for
MmBuildMdlForNonPagedPool.
> + if (ua == NULL) {
> + HCA_TRACE( HCA_DBG_ERROR, ("MmMapLockedPagesSpecifyCache
> failed.\n") );
> + IoFreeMdl( mdl_p );
> + status = STATUS_UNSUCCESSFUL;
> + goto out;
> + }
> +
> + // fill the structure
> + res_p->va = ua;
> + res_p->size = sz;
> + res_p->ctx = mdl_p;
> + status = STATUS_SUCCESS;
> +
> +out:
> + HCA_EXIT( HCA_DBG_PNP );
> + return status;
> +}
There must be some tracking of the mapped CR space to prevent two apps
from mapping the space simulatneously, as well as to unmap the space
if the app crashes or doesn't issue the unmap call - basically, if the
CA instance is closed, any mappings must be undone.
> +
> +static NTSTATUS
> +__unmap_crspace(
> + IN PVOID
> p_buf,
> + IN ULONG
> buf_size
> + )
> +{
> + NTSTATUS status;
> + PMDL mdl_p;
> + PVOID ua;
> + struct _unmap_crspace *parm = (struct _unmap_crspace *)p_buf;
> +
> + HCA_ENTER( HCA_DBG_PNP );
> +
> + // sanity checks
> + if ( buf_size < sizeof *parm ) {
> + status = STATUS_INVALID_PARAMETER;
> + goto out;
> + }
> + mdl_p = parm->ctx;
You can't trust user-mode to hand you the context. You're basically
letting a user-mode process give you a kernel pointer that you will
dereference. This is a huge security vulnerability. Better to remove
the context from the IOCTL interface all together and store it in the
CA instance.
> + ua = parm->va;
> + if ( mdl_p == NULL || ua == NULL) {
> + status = STATUS_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + // do the work
> + MmUnmapLockedPages(ua, mdl_p);
> + IoFreeMdl( mdl_p );
> + status = STATUS_SUCCESS;
> +
> +out:
> + HCA_EXIT( HCA_DBG_PNP );
> + return status;
> +}
> +
> +
> ib_api_status_t
> fw_access_ctrl(
> IN const void* __ptr64
> p_context,
> @@ -1335,6 +1440,14 @@
>
> switch ( p_ci->command )
> {
> + case FW_MAP_CRSPACE:
> + status = __map_crspace(p_hob, p_data, length);
> + break;
> +
> + case FW_UNMAP_CRSPACE:
> + status = __unmap_crspace(p_data, length);
> + break;
> +
> case FW_READ: // read data from flash
> if ( if_ready )
> {
> Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h
> ===================================================================
> --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h
> (revision 438)
> +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_data.h
> (revision 439)
> @@ -225,7 +225,12 @@
>
> typedef struct {
> cl_list_item_t list_item;
> - HH_hca_hndl_t hh_hndl;
> + HH_hca_hndl_t hh_hndl;
> + struct _hca_if {
> + HH_hca_hndl_t hh_hndl;
> + void * kernel_crspace_addr;
> + ULONG kernel_crspace_size;
Define this structure in MdCard.h just above the definition of the
GUID (but within the conditanal include #ifdef) for the interface so
that they're kept together.
> + } s;
> // char *hca_name_p;
> net64_t guid;
> const void* __ptr64 p_dev_obj; // hca device object
> @@ -373,6 +378,10 @@
> mlnx_hca_from_guid(
> IN ib_net64_t
> guid );
>
> +mlnx_hca_t*
> +mlnx_hca_from_hh_hndl(
> + IN HH_hca_hndl_t
> hh_hndl );
> +
> /*
> void
> mlnx_names_from_guid(
> Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h
> ===================================================================
> --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h
> (revision 438)
> +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_driver.h
> (revision 439)
> @@ -129,11 +129,25 @@
> #define CPUMODE_MSK (0xc0000000UL)
> #define CPUMODE_SHIFT (30)
>
> +/* buffer structure for FW_MAP_CRBASE */
> +struct _map_crspace {
> + PVOID va; /* address of CRSPACE, mapped to
> user space */
> + PVOID ctx; /* opaque operation context; to be
> used in FW_UNMAP_CRBASE */
> + ULONG size; /* size of CRSPACE, mapped to user space */
> +};
> +
> +struct _unmap_crspace {
> + PVOID va; /* address of CRSPACE, mapped to
> user space */
> + PVOID ctx; /* operation context, received in
> FW_MAP_CRBASE */
You can't trust user-mode to pass you valid kernel mode pointers.
> +};
> +
> /* Definitions intended to become shared with UM. Later... */
> #define FW_READ 0x00
> #define FW_WRITE 0x01
> #define FW_READ_CMD 0x08
> #define FW_WRITE_CMD 0x09
> +#define FW_MAP_CRSPACE 0x0A
> +#define FW_UNMAP_CRSPACE 0x0B
> #define FW_OPEN_IF 0xe7
> #define FW_CLOSE_IF 0x7e
>
>
>
>
>
>
>
>
> _______________________________________________
> openib-windows mailing list
> openib-windows at openib.org
> http://openib.org/mailman/listinfo/openib-windows
>
>
>
>
More information about the ofw
mailing list