[ofa-general] Re: [PATCH] IB/mthca: Read buffer overflow

[Roland Dreier]

 > If the QP was found in MGM in the first iteration, and we break out of
 > the loop, i == 0 and we read and write mgm->qp[-1].
 > 
 > Signed-off-by: Roel Kluin <roel.kluin at gmail.com>
 > ---
 > Not entirely sure whether it can happen

I don't think it can happen.  The loop and following code is:

	for (loc = -1, i = 0; i < MTHCA_QP_PER_MGM; ++i) {
		if (mgm->qp[i] == cpu_to_be32(ibqp->qp_num | (1 << 31)))
			loc = i;
		if (!(mgm->qp[i] & cpu_to_be32(1 << 31)))
			break;
	}

	if (loc == -1) {
		mthca_err(dev, "QP %06x not found in MGM\n", ibqp->qp_num);
		err = -EINVAL;
		goto out;
	}

	mgm->qp[loc]   = mgm->qp[i - 1];

and you're worried that i == 0 at that last bit.  For i == 0 there, we
need to break out of the loop on the first iteration, ie hit

		if (!(mgm->qp[i] & cpu_to_be32(1 << 31)))
			break;

with i == 0, meaning (mgm->qp[0] & cpu_to_be32(1 << 31) == 0.

But to get past the loc == -1 test that returns from the function, we
must also hit the loc = i assignment on the first iteration of the loop,
so we must have (mgm->qp[0] == cpu_to_be32(ibqp->qp_num | (1 << 31)) be
true, which would mean in particular that mgm->qp[0] would have to have
that high order bit set.  Which contradicts the conclusion we just
reached.

So the bad case of accessing index -1 can never happen just from the
structure of the code.

 - R.