[ofa-general] [PATCH] IB: Possible write outside array bounds

[Hal Rosenstock]

On Wed, Jul 29, 2009 at 3:22 PM, Hal Rosenstock <hal.rosenstock at gmail.com>wrote:

>
>
>   On Wed, Jul 29, 2009 at 3:15 PM, Roland Dreier <rdreier at cisco.com>wrote:
>
>>
>>  > Based on the spec limiting hop pointer to 255 and not 63, I think the
>>  > above should just be a check on hop count and not hop pointer:
>>  >             if (hop_cnt >= IB_SMP_MAX_PATH_HOPS)
>>
>> Yes, it seems that the current code then properly checks hop_ptr against
>> hop_cnt in all cases.  Do we all agree that the following patch is
>> right?  If so I'll queue it for 2.6.32:
>>
>>  drivers/infiniband/core/smi.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/smi.c b/drivers/infiniband/core/smi.c
>> index 8723675..a10152d 100644
>> --- a/drivers/infiniband/core/smi.c
>> +++ b/drivers/infiniband/core/smi.c
>> @@ -52,6 +52,10 @@ enum smi_action smi_handle_dr_smp_send(struct ib_smp
>> *smp,
>>        hop_cnt = smp->hop_cnt;
>>
>>        /* See section 14.2.2.2, Vol 1 IB spec */
>> +       /* C14-6 -- valid hop_cnt values are from 0 to 63 */
>> +       if (hop_cnt >= IB_SMP_MAX_PATH_HOPS)
>> +               return IB_SMI_DISCARD;
>> +
>>        if (!ib_get_smp_direction(smp)) {
>>                /* C14-9:1 */
>>                if (hop_cnt && hop_ptr == 0) {
>>
>
> That looks right to me on the send side. Shouldn't there be the same check
> on the recv side (smi_handle_dr_smp_recv) which was the intent of Roel's
> original patch ?
>

There's also one thing on the send side I'm not sure about. It looks to me
like c14-9:3 might break if hop_cnt is max'd out as hop_ptr is incremented
but the array is not touched.

-- Hal


>
> -- Hal
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20090729/0867c080/attachment.html>