Re: Potential use of NULL pointer in erdma/erdma_verbs.c

[Cheng Xu <chengyou@xxxxxxxxxxxxxxxxx>]

On 12/20/24 3:02 AM, Kees Bakker wrote:
> Hi,
> 
> As identified by Coverity Scan (CID 1602609) there is a potential
> use of a NULL pointer.
> 
> It was introduced in commit 6534de1fe385
> Author: Cheng Xu <chengyou@xxxxxxxxxxxxxxxxx>
> Date:   Tue Jun 6 13:50:04 2023 +0800
> 
>     RDMA/erdma: Associate QPs/CQs with doorbells for authorization
> 
>     For the isolation requirement, each QP/CQ can only issue doorbells from the
>     allocated mmio space. Configure the relationship between QPs/CQs and
>     mmio doorbell spaces to hardware in create_qp/create_cq interfaces.
> 
>     Signed-off-by: Cheng Xu <chengyou@xxxxxxxxxxxxxxxxx>
>     Link: https://lore.kernel.org/r/20230606055005.80729-4-chengyou@xxxxxxxxxxxxxxxxx
>     Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx>
> 
> int erdma_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attrs,
>                     struct ib_udata *udata)
> {
>         struct erdma_qp *qp = to_eqp(ibqp);
>         struct erdma_dev *dev = to_edev(ibqp->device);
>         struct erdma_ucontext *uctx = rdma_udata_to_drv_context(
>                 udata, struct erdma_ucontext, ibucontext);
> [...]
>         if (uctx) {
>                 ret = ib_copy_from_udata(&ureq, udata,
>                                          min(sizeof(ureq), udata->inlen));
> [...]
>         } else {
>                 init_kernel_qp(dev, qp, attrs);
>         }
> 
>         qp->attrs.max_send_sge = attrs->cap.max_send_sge;
>         qp->attrs.max_recv_sge = attrs->cap.max_recv_sge;
> 
>         if (erdma_device_iwarp(qp->dev))
>                 qp->attrs.iwarp.state = ERDMA_QPS_IWARP_IDLE;
>         else
>                 qp->attrs.rocev2.state = ERDMA_QPS_ROCEV2_RESET;
> 
>         INIT_DELAYED_WORK(&qp->reflush_dwork, erdma_flush_worker);
> 
>         ret = create_qp_cmd(uctx, qp);
> [...]
> This shows that `uctx` can be NULL. The problem is that `uctx` can be
> dereferenced in create_qp_cmd(). There is no guard against NULL.
> 
> Can one of you have a look and say that it OK to potential pass a
> NULL pointer in `uctx`?
> 

static int create_qp_cmd(struct erdma_ucontext *uctx, struct erdma_qp *qp)
{
        [...]

        if (rdma_is_kernel_res(&qp->ibqp.res)) {
            [...]
	} else {
            // uctx used here...
        }

        [...]
}

When uctx == NULL, rdma_is_kernel_res() will always return false and uctx will not be
dereferenced. So the current implementation is OK.

Thanks,
Cheng Xu


> Kind regards,
> Kees Bakker