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

[Roel Kluin]

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?

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;