Re: [PATCH for-next v11 10/13] RDMA/rxe: Stop lookup of partially built objects

[Jason Gunthorpe <jgg@nvidia.com>]

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