Potential use of NULL pointer in erdma/erdma_verbs.c

Potential use of NULL pointer in erdma/erdma_verbs.c

[Kees Bakker]

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@linux.alibaba.com>
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@linux.alibaba.com>
     Link: 
https://lore.kernel.org/r/20230606055005.80729-4-chengyou@linux.alibaba.com
     Signed-off-by: Leon Romanovsky <leon@kernel.org>

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`?

Kind regards,
Kees Bakker

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

[Cheng Xu]

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@linux.alibaba.com>
> 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@linux.alibaba.com>
>     Link: https://lore.kernel.org/r/20230606055005.80729-4-chengyou@linux.alibaba.com
>     Signed-off-by: Leon Romanovsky <leon@kernel.org>
> 
> 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

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

[Cheng Xu]

On 12/20/24 10:06 AM, Cheng Xu wrote:
> 
> 
> 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@linux.alibaba.com>
>> 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@linux.alibaba.com>
>>     Link: https://lore.kernel.org/r/20230606055005.80729-4-chengyou@linux.alibaba.com
>>     Signed-off-by: Leon Romanovsky <leon@kernel.org>
>>
>> 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
                                                             ^^^^^
                                                             true
> dereferenced. So the current implementation is OK.
> 

Sorry for this typo.

Cheng Xu

> Thanks,
> Cheng Xu
> 
> 
>> Kind regards,
>> Kees Bakker