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 <[email protected]> --- 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? 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) + 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; _______________________________________________ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
