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

Hal Rosenstock
2011-02-23T20:21:54Z

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
>>