On Thu, Mar 03, 2022 at 06:08:06PM -0600, Bob Pearson wrote:
> Currently the rdma_rxe driver has a security weakness due to giving
> objects which are partially initialized indices allowing external
> actors to gain access to them by sending packets which refer to
> their index (e.g. qpn, rkey, etc) causing unpredictable results.
>
> This patch adds two new APIs rxe_show(obj) and rxe_hide(obj) which
> enable or disable looking up pool objects from indices using
> rxe_pool_get_index(). By default objects are disabled. These APIs
> are used to enable looking up objects which have indices:
> AH, SRQ, QP, MR, and MW. They are added in create verbs after the
> objects are fully initialized and as soon as possible in destroy
> verbs.
In other parts of rdma we use the word 'finalize' where you used show
So rxe_pool_finalize() or something
I'm not clear on what hide is supposed to be for, if the object is
being destroyed why do we need a period when it is NULL in the xarray
before just erasing it?
> @@ -221,8 +221,12 @@ static void rxe_elem_release(struct kref *kref)
> {
> struct rxe_pool_elem *elem = container_of(kref, typeof(*elem), ref_cnt);
> struct rxe_pool *pool = elem->pool;
> + struct xarray *xa = &pool->xa;
> + unsigned long flags;
>
> - xa_erase(&pool->xa, elem->index);
> + xa_lock_irqsave(xa, flags);
> + __xa_erase(&pool->xa, elem->index);
> + xa_unlock_irqrestore(xa, flags);
I guess it has to do with this, but why have the xa_erase in the kref
release at all?
> if (pool->cleanup)
> pool->cleanup(elem);
> @@ -242,3 +246,33 @@ int __rxe_put(struct rxe_pool_elem *elem)
> {
> return kref_put(&elem->ref_cnt, rxe_elem_release);
> }
> +
> +int __rxe_show(struct rxe_pool_elem *elem)
> +{
> + struct xarray *xa = &elem->pool->xa;
> + unsigned long flags;
> + void *ret;
> +
> + xa_lock_irqsave(xa, flags);
> + ret = __xa_store(&elem->pool->xa, elem->index, elem, GFP_KERNEL);
> + xa_unlock_irqrestore(xa, flags);
> + if (IS_ERR(ret))
> + return PTR_ERR(ret);
This can't fail due to the xa memory already being allocated. You can
just WARN_ON here and 'finalize' should not return an error code.
If you want to be fancy this checks for other mistakes too:
tmp = xa_cmpxchg((&elem->pool->xa, elem->index, XA_ZERO_ENTRY, elem, 0)
WARN_ON(tmp != NULL);
> +int __rxe_hide(struct rxe_pool_elem *elem)
> +{
> + struct xarray *xa = &elem->pool->xa;
> + unsigned long flags;
> + void *ret;
> +
> + xa_lock_irqsave(xa, flags);
> + ret = __xa_store(&elem->pool->xa, elem->index, NULL, GFP_KERNEL);
> + xa_unlock_irqrestore(xa, flags);
IIRC storing NULL is the same as erase, isn't it? You have to store
XA_ZERO_ENTRY if you want to keep an allocated NULL
> + if (IS_ERR(ret))
> + return PTR_ERR(ret);
> + else
> + return 0;
> +}
Same remark about the error handling
> @@ -491,6 +497,7 @@ static int rxe_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
> struct rxe_qp *qp = to_rqp(ibqp);
> int ret;
>
> + rxe_hide(qp);
> ret = rxe_qp_chk_destroy(qp);
> if (ret)
> return ret;
So we decided not to destroy the QP but wrecked it in the xarray?
Not convinced about the hide at all..
Jason
