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

[Hal Rosenstock]

On Fri, Jul 24, 2009 at 11:45 AM, Roel Kluin<roel.kluin at gmail.com> wrote:
> Ensure index stays within smp->return_path[] and ->initial_path[].
> A hop_cnt or hop_ptr greater or equal to IB_SMP_MAX_PATH_HOPS is invalid.
>
> Signed-off-by: Roel Kluin <roel.kluin at gmail.com>
> ---
> Observed using Parfait, http://research.sun.com/projects/parfait/
>
> Oops, restoring CCs, this was the discussion:
>
>>>> Yes, nice catch. This appears correct but I think the issue of hop
>>>> path/count validation is slightly larger. In that routine
>>>> (smi_handle_dr_smp_recv), I would think that if either hop_ptr or
>>>> hop_cnt are >= IB_SMP_MAX_PATH_HOPS then IB_SMI_DISCARD should be
>>>> returned. If that validation is done at the top of that routine, then
>>>> the hop_ptr < hop_cnt check in this C14-9:2 case eliminates the need
>>>> to special case this here. Make sense ?
>>> Some, but there may be more to be considered:
>>>
>>> * In C14-13:1 and C14-13:2 a decrement occurs before accessing the
>>> smp->return_path[] element. Therefore an initial smp->hop_ptr of
>>> SMP_MAX_PATH_HOPS could be valid (not sure). But implimenting
>>> the test at the top of that routine will cause a return of IB_SMI_DISCARD.
>>
>> Is your point that if hop_ptr of MAX_PATH_HOPS is valid there, then
>> returning IB_SMI_DISCARD is incorrect ?
>
> The point was that I wasn't sure.
>
>> When this routine is called, hop_ptr and hop_cnt are as received from
>> the IB wire.
>>
>> In C14-13:2, hop_ptr <= hop_cnt and hop_cnt of MAX_PATH_HOPS is
>> invalid so this is safe.
>>
>> In C14-13:1, hop_ptr == hop_cnt + 1 does appear to allow hop_ptr to be
>> MAX_PATH_HOPS there.
>>
>> So I now think your check is needed as well as adding a validation of hop_cnt
>> at the top of the routine as valid values are 0 to 63 as indicated in C14-6.
>
> So I see...
>
>> The good news is this is all theoretical as no one has come close to
>> building a depth 64 IB subnet.
>>
>> -- Hal
>
> Thanks for your review. I hope this was as you intended?

Close; see below.

> diff --git a/drivers/infiniband/core/smi.c b/drivers/infiniband/core/smi.c
> index 8723675..9d66c42 100644
> --- a/drivers/infiniband/core/smi.c
> +++ b/drivers/infiniband/core/smi.c
> @@ -132,6 +132,9 @@ enum smi_action smi_handle_dr_smp_recv(struct ib_smp *smp, u8 node_type,
>        hop_ptr = smp->hop_ptr;
>        hop_cnt = smp->hop_cnt;
>
> +       if (hop_ptr >= IB_SMP_MAX_PATH_HOPS || hop_cnt >= IB_SMP_MAX_PATH_HOPS)

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)

-- Hal

> +               return IB_SMI_DISCARD;
> +
>        /* See section 14.2.2.2, Vol 1 IB spec */
>        if (!ib_get_smp_direction(smp)) {
>                /* C14-9:1 -- sender should have incremented hop_ptr */
> @@ -140,7 +143,8 @@ enum smi_action smi_handle_dr_smp_recv(struct ib_smp *smp, u8 node_type,
>
>                /* C14-9:2 -- intermediate hop */
>                if (hop_ptr && hop_ptr < hop_cnt) {
> -                       if (node_type != RDMA_NODE_IB_SWITCH)
> +                       if (node_type != RDMA_NODE_IB_SWITCH ||
> +                                       hop_ptr + 1 >= IB_SMP_MAX_PATH_HOPS)
>                                return IB_SMI_DISCARD;
>
>                        smp->return_path[hop_ptr] = port_num;
>