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

[Hal Rosenstock]

On Wed, Jul 29, 2009 at 3:34 PM, 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>
> ---
> Op 29-07-09 21:22, Hal Rosenstock schreef:
> >
> > On Wed, Jul 29, 2009 at 3:15 PM, Roland Dreier <rdreier at cisco.com
> > <mailto: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:
>
> > 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 ?
> >
> > -- Hal
> >
>
> I think we also need the `hop_ptr + 1 >= IB_SMP_MAX_PATH_HOPS' test, right?


Where are you referring to add this ? Like in your original patch ?

-- Hal


>
>
> diff --git a/drivers/infiniband/core/smi.c b/drivers/infiniband/core/smi.c
> index 8723675..c623da7 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) {
> @@ -132,6 +136,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_cnt >= IB_SMP_MAX_PATH_HOPS)
> +               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 +147,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;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20090729/9372afa4/attachment.html>