IB/uverbs: multiple possible overflows

Hi,

There are multiple integer overflows that may lead to buffer overflows in drivers/infiniband/core/uverbs_cmd.c. I will explain how an exploit might work and suggest some patches. Thanks for reviewing.

Possible exploit
================

Consider ib_uverbs_unmarshall_recv(). The function is called from ib_uverbs_post_recv() and ib_uverbs_post_srq_recv(). In both cases, its parameters wr_count, sge_count, and wqe_size are directly from userspace via copy_from_user() and thus they can be any values.

Let's choose the following values, and see how to bypass the existing checks and reach "next = kmalloc(...)". Assume 32-bit systems.

        wr_count = 1
        sge_count = 0x80000000
        wqe_size = 16

1. NOT in_len < wqe_size * wr_count + sge_count * 16

if (in_len < wqe_size * wr_count +
sge_count * sizeof (struct ib_uverbs_sge))
return ERR_PTR(-EINVAL);

Here struct ib_uverbs_sge is 16 bytes.

Since the second multiplication 0x80000000 * 16 overflows and becomes 0, the check is basically in_len < 16, which can be easily bypassed.

2. NOT wqe_size < 16

if (wqe_size < sizeof (struct ib_uverbs_recv_wr))
return ERR_PTR(-EINVAL);

Here struct ib_uverbs_recv_wr is 16 bytes.

Since we choose wqe_size = 16, the check will be bypassed.

3. wqe_size <= KMALLOC_MAX_SIZE

        user_wr = kmalloc(wqe_size, GFP_KERNEL);
        if (!user_wr)
                return ERR_PTR(-ENOMEM);

To make kmalloc() succeed, wqe_size shouldn't be large. Again, since we choose wqe_size = 16, the check will be bypassed.

4. wr_count > 0

        for (i = 0; i < wr_count; ++i) { ... }

Since we choose wr_count = 1, the loop will run once.

5. NOT num_sge + sg_ind > sge_count

        if (user_wr->num_sge + sg_ind > sge_count) {
                ret = -EINVAL;
                goto err;
        }

Here sg_ind is 0. Note that num_sge is also from userspace via copy_from_user(). Let's choose num_sge = sge_count = 0x80000000, so as to bypass the check.

Now we come to the allocation call.

        next = kmalloc(ALIGN(sizeof *next, sizeof (struct ib_sge)) +
                       user_wr->num_sge * sizeof (struct ib_sge),
                       GFP_KERNEL);

The allocation size is basically 32 + num_sge * 16 = 32, which is smaller than expected.

Also note that in

	next->num_sg = user_wr->num_sge;

next->num_sge (ib_recv_wr::num_sge) is signed.

        next->sg_list = (void *) next +
                ALIGN(sizeof *next, sizeof (struct ib_sge));

Now next->sg_list points to the end of the allocated memory. Any access to sg_list[i] would be out of bounds.

The subsequent copy_from_user(next->sg_list, ...) is a no-op, since the size next->num_sge * sizeof (struct ib_sge) evaluates to 0.

Now ib_uverbs_unmarshall_recv() returns to its caller with a negative wr->num_sge = 0x80000000 and wr->sg_list that points to a bad location.

Let's look at one of the callers, ib_uverbs_post_recv(). The function then calls into a specific ->post_recv().

        qp->device->post_recv(qp->real_qp, wr, &bad_wr);

We use iwch_post_receive() in drivers/infiniband/hw/cxgb3/iwch_qp.c as an example.

        if (wr->num_sge > T3_MAX_SGE) {
                err = -EINVAL;
                break;
        }
        ...
        if (num_wrs)
                if (wr->sg_list[0].lkey)
                        err = build_rdma_recv(qhp, wqe, wr);
                else
                        err = build_zero_stag_recv(qhp, wqe, wr);
        else
                err = -ENOMEM;

The check "wr->num_sge > T3_MAX_SGE" will be bypassed since wr->num_sge is negative 0x80000000.

The access "wr->sg_list[0].lkey" is out of bounds. Not sure what value it gets. We could have bigger trouble in build_rdma_recv(), because it calls iwch_sgl2pbl_map() with wr->num_sge.

        for (i = 0; i < num_sgle; i++) {
                ...
                pbl_addr[i] = ...;
                page_size[i] = ...;
        }

This time num_sgle = 0x80000000 is interpreted as unsigned (u32), and the loop may overwrite a lot of places.

Other overflows
===============

ib_uverbs_post_send() has similar code.

Suggested patches
=================

1. Change the types of the following struct fields to unsigned.

        ib_recv_wr::num_sge
        ib_send_wr::num_sge

This ensures that driver-specific checks like wr->num_sge > T3_MAX_SGE won't be bypassed.

2. Fix ib_uverbs_unmarshall_recv() and ib_uverbs_post_send().

I am not sure if there's any well-known or reasonable constant limit for every value from userspace:

wr_count
sge_count
wqe_size
num_sge

If we know such limits, define them as something like WR_COUNT_MAX for checking.

If the limits do not exist, then it might be boring to fix the code. For example, in addition to

if (in_len < wqe_size * wr_count +
sge_count * sizeof (struct ib_uverbs_sge))
return ERR_PTR(-EINVAL);

We might have to write many more overflow checks:

        if (wr_count && wqe_size > UINT_MAX / wr_count)
            return ERR_PTR(-EINVAL);

        if (sge_count > (UINT_MAX - wqe_size * wr_count)
                        / sizeof (struct ib_uverbs_sge))
            return ERR_PTR(-EINVAL);

- xi
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html