[Openib-windows] [PATCH] 2 new vendor calls
Leonid Keller
leonid at mellanox.co.il
Thu Sep 22 01:08:31 PDT 2005
Hi Fab,
Thank you for your comments.
First of all, the problem with unmapping is in need in releasing of the MDL,
not in unmapping itself.
You are right about security and resource tracking: i saw these problems,
but didn't find a simple solution.
The right solution IMO would have been to add the context of FW_MAP_CRSPACE
operation to the process resource tracking information and to perform
FW_UNMAP_CRSPACE automatically upon process termination.
Then we could remove the context from FW_MAP_CRSPACE and not provide
FW_UNMAP_CRSPACE at all.
Your idea of putting the context into CA object seams to me troublesome:
- it prevents several applications to use simultaneously this
mechanism (on the same HCA);
- if an application was killed and restarted, there will be a memory
leak.
I don't know enough how the resource tracking mechaism works today and
whether there is a simple way to implement the above solution.
Could you enlight me on this ?
TIA
> -----Original Message-----
> From: Fabian Tillier [mailto:ftillier at silverstorm.com]
> Sent: Wednesday, September 21, 2005 10:00 PM
> To: Leonid Keller
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] [PATCH] 2 new vendor calls
>
>
> 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
> >
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20050922/b100ae9c/attachment.html>
More information about the ofw
mailing list