[PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Jason Gunthorpe]

* Properly byteswap block_num before using it
* Properly check bounds of the block number before de-referencing it.

This fixes a remotely triggered null pointer dereference.
---
 opensm/include/iba/ib_types.h      |    2 +-
 opensm/opensm/osm_sa_pkey_record.c |   16 ++++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
index e1bc102..24f5662 100644
--- a/opensm/include/iba/ib_types.h
+++ b/opensm/include/iba/ib_types.h
@@ -6724,7 +6724,7 @@ typedef struct _ib_pkey_table {
 #include <complib/cl_packon.h>
 typedef struct _ib_pkey_table_record {
 	ib_net16_t lid;		// for CA: lid of port, for switch lid of port 0
-	uint16_t block_num;
+	ib_net16_t block_num;
 	uint8_t port_num;	// for switch: port number, for CA: reserved
 	uint8_t reserved1;
 	uint16_t reserved2;
diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
index e4930d0..cf50430 100644
--- a/opensm/opensm/osm_sa_pkey_record.c
+++ b/opensm/opensm/osm_sa_pkey_record.c
@@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
 	osm_pkey_item_t *p_rec_item;
 	uint16_t lid;
 	ib_api_status_t status = IB_SUCCESS;
+	ib_pkey_table_t *tbl;
 
 	OSM_LOG_ENTER(sa->p_log);
 
@@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
 	p_rec_item->rec.lid = lid;
 	p_rec_item->rec.block_num = block;
 	p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
-	p_rec_item->rec.pkey_tbl =
-	    *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
+	/* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
+	   pkey entries so everything in that range is a valid block number
+	   even if opensm is not using it. Return 0. However things outside
+	   that range should return no entries.. Not sure how to figure that
+	   here? The range of pkey_tbl can be less than the cap, so
+	   this falsely triggers. */
+	tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
+	if (tbl)
+		p_rec_item->rec.pkey_tbl = *tbl;
 
 	cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);
 
@@ -269,14 +277,14 @@ void osm_pkey_rec_rcv_process(IN void *ctx, IN void *data)
 	context.p_list = &rec_list;
 	context.comp_mask = p_rcvd_mad->comp_mask;
 	context.sa = sa;
-	context.block_num = p_rcvd_rec->block_num;
+	context.block_num = cl_ntoh16(p_rcvd_rec->block_num);
 	context.p_req_physp = p_req_physp;
 
 	OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
 		"Got Query Lid:%u(%02X), Block:0x%02X(%02X), Port:0x%02X(%02X)\n",
 		cl_ntoh16(p_rcvd_rec->lid),
 		(comp_mask & IB_PKEY_COMPMASK_LID) != 0, p_rcvd_rec->port_num,
-		(comp_mask & IB_PKEY_COMPMASK_PORT) != 0, p_rcvd_rec->block_num,
+		(comp_mask & IB_PKEY_COMPMASK_PORT) != 0, context.block_num,
 		(comp_mask & IB_PKEY_COMPMASK_BLOCK) != 0);
 
 	cl_plock_acquire(sa->p_lock);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Alex Netes]

Hi Jason,

On 15:12 Fri 11 Feb     , Jason Gunthorpe wrote:
> * Properly byteswap block_num before using it
> * Properly check bounds of the block number before de-referencing it.
> 
> This fixes a remotely triggered null pointer dereference.
> ---
>  opensm/include/iba/ib_types.h      |    2 +-
>  opensm/opensm/osm_sa_pkey_record.c |   16 ++++++++++++----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
> index e1bc102..24f5662 100644
> --- a/opensm/include/iba/ib_types.h
> +++ b/opensm/include/iba/ib_types.h
> @@ -6724,7 +6724,7 @@ typedef struct _ib_pkey_table {
>  #include <complib/cl_packon.h>
>  typedef struct _ib_pkey_table_record {
>  	ib_net16_t lid;		// for CA: lid of port, for switch lid of port 0
> -	uint16_t block_num;
> +	ib_net16_t block_num;
>  	uint8_t port_num;	// for switch: port number, for CA: reserved
>  	uint8_t reserved1;
>  	uint16_t reserved2;

I guess reserved2 could also be changed to ib_net16_t, right?

> diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
> index e4930d0..cf50430 100644
> --- a/opensm/opensm/osm_sa_pkey_record.c
> +++ b/opensm/opensm/osm_sa_pkey_record.c
> @@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>  	osm_pkey_item_t *p_rec_item;
>  	uint16_t lid;
>  	ib_api_status_t status = IB_SUCCESS;
> +	ib_pkey_table_t *tbl;
>  
>  	OSM_LOG_ENTER(sa->p_log);
>  
> @@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>  	p_rec_item->rec.lid = lid;
>  	p_rec_item->rec.block_num = block;
>  	p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
> -	p_rec_item->rec.pkey_tbl =
> -	    *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
> +	/* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
> +	   pkey entries so everything in that range is a valid block number
> +	   even if opensm is not using it. Return 0. However things outside
> +	   that range should return no entries.. Not sure how to figure that
> +	   here? The range of pkey_tbl can be less than the cap, so
> +	   this falsely triggers. */
> +	tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
> +	if (tbl)
> +		p_rec_item->rec.pkey_tbl = *tbl;
>  
>  	cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);

What is the expected behaviour when IB PKey table block is empty?
rec.pkey_tbl might be uninitialized here. 
Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?

>  
> @@ -269,14 +277,14 @@ void osm_pkey_rec_rcv_process(IN void *ctx, IN void *data)
>  	context.p_list = &rec_list;
>  	context.comp_mask = p_rcvd_mad->comp_mask;
>  	context.sa = sa;
> -	context.block_num = p_rcvd_rec->block_num;
> +	context.block_num = cl_ntoh16(p_rcvd_rec->block_num);
>  	context.p_req_physp = p_req_physp;
>  
>  	OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
>  		"Got Query Lid:%u(%02X), Block:0x%02X(%02X), Port:0x%02X(%02X)\n",
>  		cl_ntoh16(p_rcvd_rec->lid),
>  		(comp_mask & IB_PKEY_COMPMASK_LID) != 0, p_rcvd_rec->port_num,
> -		(comp_mask & IB_PKEY_COMPMASK_PORT) != 0, p_rcvd_rec->block_num,
> +		(comp_mask & IB_PKEY_COMPMASK_PORT) != 0, context.block_num,
>  		(comp_mask & IB_PKEY_COMPMASK_BLOCK) != 0);
>  
>  	cl_plock_acquire(sa->p_lock);
> -- 
> 1.7.1
> 

Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Jason Gunthorpe]

On Wed, Feb 23, 2011 at 07:20:19PM +0200, Alex Netes wrote:

> I guess reserved2 could also be changed to ib_net16_t, right?

Sure, but unrelated, might be lots more too?
 
> > diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
> > index e4930d0..cf50430 100644
> > +++ b/opensm/opensm/osm_sa_pkey_record.c
> > @@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
> >  	osm_pkey_item_t *p_rec_item;
> >  	uint16_t lid;
> >  	ib_api_status_t status = IB_SUCCESS;
> > +	ib_pkey_table_t *tbl;
> >  
> >  	OSM_LOG_ENTER(sa->p_log);
> >  
> > @@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
> >  	p_rec_item->rec.lid = lid;
> >  	p_rec_item->rec.block_num = block;
> >  	p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
> > -	p_rec_item->rec.pkey_tbl =
> > -	    *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
> > +	/* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
> > +	   pkey entries so everything in that range is a valid block number
> > +	   even if opensm is not using it. Return 0. However things outside
> > +	   that range should return no entries.. Not sure how to figure that
> > +	   here? The range of pkey_tbl can be less than the cap, so
> > +	   this falsely triggers. */
> > +	tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
> > +	if (tbl)
> > +		p_rec_item->rec.pkey_tbl = *tbl;
> >  
> >  	cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);
>
> What is the expected behaviour when IB PKey table block is empty?

IBA is unclear in this case. In my view, direct queries for SA records
should return the same result as a direct SMP for the same data, so
returning a wack of 0 is appropriate. Another view would be that the
SA data is 'empty' so ERR_NO_RECORDS is OK, which matches what
GetTable would return.

> rec.pkey_tbl might be uninitialized here.

All of p_rec_item is memset to 0.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Hal Rosenstock]

On Wed, Feb 23, 2011 at 12:20 PM, Alex Netes <alexne-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> Hi Jason,
>
> On 15:12 Fri 11 Feb     , Jason Gunthorpe wrote:
>> * Properly byteswap block_num before using it
>> * Properly check bounds of the block number before de-referencing it.
>>
>> This fixes a remotely triggered null pointer dereference.
>> ---
>>  opensm/include/iba/ib_types.h      |    2 +-
>>  opensm/opensm/osm_sa_pkey_record.c |   16 ++++++++++++----
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
>> index e1bc102..24f5662 100644
>> --- a/opensm/include/iba/ib_types.h
>> +++ b/opensm/include/iba/ib_types.h
>> @@ -6724,7 +6724,7 @@ typedef struct _ib_pkey_table {
>>  #include <complib/cl_packon.h>
>>  typedef struct _ib_pkey_table_record {
>>       ib_net16_t lid;         // for CA: lid of port, for switch lid of port 0
>> -     uint16_t block_num;
>> +     ib_net16_t block_num;
>>       uint8_t port_num;       // for switch: port number, for CA: reserved
>>       uint8_t reserved1;
>>       uint16_t reserved2;
>
> I guess reserved2 could also be changed to ib_net16_t, right?
>
>> diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
>> index e4930d0..cf50430 100644
>> --- a/opensm/opensm/osm_sa_pkey_record.c
>> +++ b/opensm/opensm/osm_sa_pkey_record.c
>> @@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>>       osm_pkey_item_t *p_rec_item;
>>       uint16_t lid;
>>       ib_api_status_t status = IB_SUCCESS;
>> +     ib_pkey_table_t *tbl;
>>
>>       OSM_LOG_ENTER(sa->p_log);
>>
>> @@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>>       p_rec_item->rec.lid = lid;
>>       p_rec_item->rec.block_num = block;
>>       p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
>> -     p_rec_item->rec.pkey_tbl =
>> -         *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
>> +     /* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
>> +        pkey entries so everything in that range is a valid block number
>> +        even if opensm is not using it. Return 0. However things outside
>> +        that range should return no entries.. Not sure how to figure that
>> +        here? The range of pkey_tbl can be less than the cap, so
>> +        this falsely triggers. */
>> +     tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
>> +     if (tbl)
>> +             p_rec_item->rec.pkey_tbl = *tbl;
>>
>>       cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);
>
> What is the expected behaviour when IB PKey table block is empty?
> rec.pkey_tbl might be uninitialized here.
> Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?

It depends on the method used in the SA query. GetTable never returns
ERR_NO_RECORDS whereas a Get can.

-- Hal

>>
>> @@ -269,14 +277,14 @@ void osm_pkey_rec_rcv_process(IN void *ctx, IN void *data)
>>       context.p_list = &rec_list;
>>       context.comp_mask = p_rcvd_mad->comp_mask;
>>       context.sa = sa;
>> -     context.block_num = p_rcvd_rec->block_num;
>> +     context.block_num = cl_ntoh16(p_rcvd_rec->block_num);
>>       context.p_req_physp = p_req_physp;
>>
>>       OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
>>               "Got Query Lid:%u(%02X), Block:0x%02X(%02X), Port:0x%02X(%02X)\n",
>>               cl_ntoh16(p_rcvd_rec->lid),
>>               (comp_mask & IB_PKEY_COMPMASK_LID) != 0, p_rcvd_rec->port_num,
>> -             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, p_rcvd_rec->block_num,
>> +             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, context.block_num,
>>               (comp_mask & IB_PKEY_COMPMASK_BLOCK) != 0);
>>
>>       cl_plock_acquire(sa->p_lock);
>> --
>> 1.7.1
>>
>
> Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Hal Rosenstock]

On Wed, Feb 23, 2011 at 2:01 PM, Hal Rosenstock
<hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Feb 23, 2011 at 12:20 PM, Alex Netes <alexne-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
>> Hi Jason,
>>
>> On 15:12 Fri 11 Feb     , Jason Gunthorpe wrote:
>>> * Properly byteswap block_num before using it
>>> * Properly check bounds of the block number before de-referencing it.
>>>
>>> This fixes a remotely triggered null pointer dereference.
>>> ---
>>>  opensm/include/iba/ib_types.h      |    2 +-
>>>  opensm/opensm/osm_sa_pkey_record.c |   16 ++++++++++++----
>>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
>>> index e1bc102..24f5662 100644
>>> --- a/opensm/include/iba/ib_types.h
>>> +++ b/opensm/include/iba/ib_types.h
>>> @@ -6724,7 +6724,7 @@ typedef struct _ib_pkey_table {
>>>  #include <complib/cl_packon.h>
>>>  typedef struct _ib_pkey_table_record {
>>>       ib_net16_t lid;         // for CA: lid of port, for switch lid of port 0
>>> -     uint16_t block_num;
>>> +     ib_net16_t block_num;
>>>       uint8_t port_num;       // for switch: port number, for CA: reserved
>>>       uint8_t reserved1;
>>>       uint16_t reserved2;
>>
>> I guess reserved2 could also be changed to ib_net16_t, right?
>>
>>> diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
>>> index e4930d0..cf50430 100644
>>> --- a/opensm/opensm/osm_sa_pkey_record.c
>>> +++ b/opensm/opensm/osm_sa_pkey_record.c
>>> @@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>>>       osm_pkey_item_t *p_rec_item;
>>>       uint16_t lid;
>>>       ib_api_status_t status = IB_SUCCESS;
>>> +     ib_pkey_table_t *tbl;
>>>
>>>       OSM_LOG_ENTER(sa->p_log);
>>>
>>> @@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
>>>       p_rec_item->rec.lid = lid;
>>>       p_rec_item->rec.block_num = block;
>>>       p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
>>> -     p_rec_item->rec.pkey_tbl =
>>> -         *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
>>> +     /* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
>>> +        pkey entries so everything in that range is a valid block number
>>> +        even if opensm is not using it. Return 0. However things outside
>>> +        that range should return no entries.. Not sure how to figure that
>>> +        here? The range of pkey_tbl can be less than the cap, so
>>> +        this falsely triggers. */
>>> +     tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
>>> +     if (tbl)
>>> +             p_rec_item->rec.pkey_tbl = *tbl;
>>>
>>>       cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);
>>
>> What is the expected behaviour when IB PKey table block is empty?
>> rec.pkey_tbl might be uninitialized here.
>> Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?
>
> It depends on the method used in the SA query. GetTable never returns
> ERR_NO_RECORDS whereas a Get can.

I'm referring to a block that is fully above the PartitionCap in the
above. For a block included by the PartitionCap, there's should always
be a "valid" block to be returned even if the entries indicate invalid
pkey.

-- Hal

>
> -- Hal
>
>>>
>>> @@ -269,14 +277,14 @@ void osm_pkey_rec_rcv_process(IN void *ctx, IN void *data)
>>>       context.p_list = &rec_list;
>>>       context.comp_mask = p_rcvd_mad->comp_mask;
>>>       context.sa = sa;
>>> -     context.block_num = p_rcvd_rec->block_num;
>>> +     context.block_num = cl_ntoh16(p_rcvd_rec->block_num);
>>>       context.p_req_physp = p_req_physp;
>>>
>>>       OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
>>>               "Got Query Lid:%u(%02X), Block:0x%02X(%02X), Port:0x%02X(%02X)\n",
>>>               cl_ntoh16(p_rcvd_rec->lid),
>>>               (comp_mask & IB_PKEY_COMPMASK_LID) != 0, p_rcvd_rec->port_num,
>>> -             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, p_rcvd_rec->block_num,
>>> +             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, context.block_num,
>>>               (comp_mask & IB_PKEY_COMPMASK_BLOCK) != 0);
>>>
>>>       cl_plock_acquire(sa->p_lock);
>>> --
>>> 1.7.1
>>>
>>
>> Alex
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Alex Netes]

On 14:01 Wed 23 Feb     , Hal Rosenstock wrote:
> On Wed, Feb 23, 2011 at 12:20 PM, Alex Netes <alexne-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> > Hi Jason,
> >
> > On 15:12 Fri 11 Feb     , Jason Gunthorpe wrote:
> >> * Properly byteswap block_num before using it
> >> * Properly check bounds of the block number before de-referencing it.
> >>
> >> This fixes a remotely triggered null pointer dereference.
> >> ---
> >>  opensm/include/iba/ib_types.h      |    2 +-
> >>  opensm/opensm/osm_sa_pkey_record.c |   16 ++++++++++++----
> >>  2 files changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
> >> index e1bc102..24f5662 100644
> >> --- a/opensm/include/iba/ib_types.h
> >> +++ b/opensm/include/iba/ib_types.h
> >> @@ -6724,7 +6724,7 @@ typedef struct _ib_pkey_table {
> >>  #include <complib/cl_packon.h>
> >>  typedef struct _ib_pkey_table_record {
> >>       ib_net16_t lid;         // for CA: lid of port, for switch lid of port 0
> >> -     uint16_t block_num;
> >> +     ib_net16_t block_num;
> >>       uint8_t port_num;       // for switch: port number, for CA: reserved
> >>       uint8_t reserved1;
> >>       uint16_t reserved2;
> >
> > I guess reserved2 could also be changed to ib_net16_t, right?
> >
> >> diff --git a/opensm/opensm/osm_sa_pkey_record.c b/opensm/opensm/osm_sa_pkey_record.c
> >> index e4930d0..cf50430 100644
> >> --- a/opensm/opensm/osm_sa_pkey_record.c
> >> +++ b/opensm/opensm/osm_sa_pkey_record.c
> >> @@ -71,6 +71,7 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
> >>       osm_pkey_item_t *p_rec_item;
> >>       uint16_t lid;
> >>       ib_api_status_t status = IB_SUCCESS;
> >> +     ib_pkey_table_t *tbl;
> >>
> >>       OSM_LOG_ENTER(sa->p_log);
> >>
> >> @@ -98,8 +99,15 @@ static void sa_pkey_create(IN osm_sa_t * sa, IN osm_physp_t * p_physp,
> >>       p_rec_item->rec.lid = lid;
> >>       p_rec_item->rec.block_num = block;
> >>       p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
> >> -     p_rec_item->rec.pkey_tbl =
> >> -         *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block));
> >> +     /* FIXME: There are ninf.PartitionCap or swinf.PartitionEnforcementCap
> >> +        pkey entries so everything in that range is a valid block number
> >> +        even if opensm is not using it. Return 0. However things outside
> >> +        that range should return no entries.. Not sure how to figure that
> >> +        here? The range of pkey_tbl can be less than the cap, so
> >> +        this falsely triggers. */
> >> +     tbl = osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp), block);
> >> +     if (tbl)
> >> +             p_rec_item->rec.pkey_tbl = *tbl;
> >>
> >>       cl_qlist_insert_tail(p_ctxt->p_list, &p_rec_item->list_item);
> >
> > What is the expected behaviour when IB PKey table block is empty?
> > rec.pkey_tbl might be uninitialized here.
> > Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?
> 
> It depends on the method used in the SA query. GetTable never returns
> ERR_NO_RECORDS whereas a Get can.
> 

In case when IB PKey table block is empty, p_rec_item->rec.pkey_tbl would be uninitialized.

> -- Hal
> 
> >>
> >> @@ -269,14 +277,14 @@ void osm_pkey_rec_rcv_process(IN void *ctx, IN void *data)
> >>       context.p_list = &rec_list;
> >>       context.comp_mask = p_rcvd_mad->comp_mask;
> >>       context.sa = sa;
> >> -     context.block_num = p_rcvd_rec->block_num;
> >> +     context.block_num = cl_ntoh16(p_rcvd_rec->block_num);
> >>       context.p_req_physp = p_req_physp;
> >>
> >>       OSM_LOG(sa->p_log, OSM_LOG_DEBUG,
> >>               "Got Query Lid:%u(%02X), Block:0x%02X(%02X), Port:0x%02X(%02X)\n",
> >>               cl_ntoh16(p_rcvd_rec->lid),
> >>               (comp_mask & IB_PKEY_COMPMASK_LID) != 0, p_rcvd_rec->port_num,
> >> -             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, p_rcvd_rec->block_num,
> >> +             (comp_mask & IB_PKEY_COMPMASK_PORT) != 0, context.block_num,
> >>               (comp_mask & IB_PKEY_COMPMASK_BLOCK) != 0);
> >>
> >>       cl_plock_acquire(sa->p_lock);
> >> --
> >> 1.7.1
> >>
> >
> > Alex
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Jason Gunthorpe]

On Fri, Feb 25, 2011 at 01:25:15PM +0200, Alex Netes wrote:

> > > What is the expected behaviour when IB PKey table block is empty?
> > > rec.pkey_tbl might be uninitialized here.
> > > Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?
> > 
> > It depends on the method used in the SA query. GetTable never returns
> > ERR_NO_RECORDS whereas a Get can.
> 
> In case when IB PKey table block is empty, p_rec_item->rec.pkey_tbl would be uninitialized.

No, it isn't, it is zero.

        memset(p_rec_item, 0, sizeof(*p_rec_item));

        p_rec_item->rec.lid = lid;
        p_rec_item->rec.block_num = block;
        p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
        p_rec_item->rec.pkey_tbl =
            *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp),
	    block));


Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Alex Netes]

On 12:35 Fri 25 Feb     , Jason Gunthorpe wrote:
> On Fri, Feb 25, 2011 at 01:25:15PM +0200, Alex Netes wrote:
> 
> > > > What is the expected behaviour when IB PKey table block is empty?
> > > > rec.pkey_tbl might be uninitialized here.
> > > > Shouldn't SubnAdmGetResp contain ERR_NO_RECORDS in such case?
> > > 
> > > It depends on the method used in the SA query. GetTable never returns
> > > ERR_NO_RECORDS whereas a Get can.
> > 
> > In case when IB PKey table block is empty, p_rec_item->rec.pkey_tbl would be uninitialized.
> 
> No, it isn't, it is zero.
> 
>         memset(p_rec_item, 0, sizeof(*p_rec_item));
> 
>         p_rec_item->rec.lid = lid;
>         p_rec_item->rec.block_num = block;
>         p_rec_item->rec.port_num = osm_physp_get_port_num(p_physp);
>         p_rec_item->rec.pkey_tbl =
>             *(osm_pkey_tbl_block_get(osm_physp_get_pkey_tbl(p_physp),
> 	    block));
> 
> 
> Jason

Sorry. Missed that.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/opensm] Decode the SAPKeyTableRecord block number properly, and don't segv

[Alex Netes]

On 15:12 Fri 11 Feb     , Jason Gunthorpe wrote:
> * Properly byteswap block_num before using it
> * Properly check bounds of the block number before de-referencing it.
> 
> This fixes a remotely triggered null pointer dereference.
> ---

Applied. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html