[bug report] RDMA/mana_ib: implement uapi for creation of rnic cq

[bug report] RDMA/mana_ib: implement uapi for creation of rnic cq

[Dan Carpenter]

Hello Konstantin Taranov,

Commit 44b607ad4cdf ("RDMA/mana_ib: implement uapi for creation of
rnic cq") from Apr 26, 2024 (linux-next), leads to the following
Smatch static checker warning:

	drivers/infiniband/hw/mana/cq.c:48 mana_ib_create_cq()
	warn: potential user controlled sizeof overflow 'cq->cqe * 64' 's32min-s32max * 64'

drivers/infiniband/hw/mana/cq.c
    8 int mana_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
    9                       struct uverbs_attr_bundle *attrs)
    10 {
    11         struct ib_udata *udata = &attrs->driver_udata;
    12         struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
    13         struct mana_ib_create_cq_resp resp = {};
    14         struct mana_ib_ucontext *mana_ucontext;
    15         struct ib_device *ibdev = ibcq->device;
    16         struct mana_ib_create_cq ucmd = {};
    17         struct mana_ib_dev *mdev;
    18         struct gdma_context *gc;
    19         bool is_rnic_cq;
    20         u32 doorbell;
    21         u32 buf_size;
    22         int err;
    23 
    24         mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
    25         gc = mdev_to_gc(mdev);
    26 
    27         cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
    28         cq->cq_handle = INVALID_MANA_HANDLE;
    29 
    30         if (udata) {
    31                 if (udata->inlen < offsetof(struct mana_ib_create_cq, flags))
    32                         return -EINVAL;
    33 
    34                 err = ib_copy_from_udata(&ucmd, udata, min(sizeof(ucmd), udata->inlen));
                                                 ^^^^
ucmd.flags is set by the user here.

    35                 if (err) {
    36                         ibdev_dbg(ibdev, "Failed to copy from udata for create cq, %d\n", err);
    37                         return err;
    38                 }
    39 
    40                 is_rnic_cq = !!(ucmd.flags & MANA_IB_CREATE_RNIC_CQ);
    41 
    42                 if (!is_rnic_cq && attr->cqe > mdev->adapter_caps.max_qp_wr) {
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
attr->cqe used to be bounds checked every time, but now the user can
skip setting the flag for bounds checking.

    43                         ibdev_dbg(ibdev, "CQE %d exceeding limit\n", attr->cqe);
    44                         return -EINVAL;
    45                 }
    46 
    47                 cq->cqe = attr->cqe;
--> 48                 err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe * COMP_ENTRY_SIZE,
                                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^

This can lead to integer wrapping.

The call tree is:

ib_uverbs_create_cq() <- copies cmd.cqe from the user
  -> create_cq() calls (struct ib_device_ops)->create_cq()
     -> mana_ib_create_cq()

I'm not sure if this integer overflow has any negative effects.  I think
it's probably fine?


    49                                            &cq->queue);
    50                 if (err) {
    51                         ibdev_dbg(ibdev, "Failed to create queue for create cq, %d\n", err);
    52                         return err;
    53                 }
    54 
    55                 mana_ucontext = rdma_udata_to_drv_context(udata, struct mana_ib_ucontext,
    56                                                           ibucontext);
    57                 doorbell = mana_ucontext->doorbell;
    58         } else {
    59                 is_rnic_cq = true;
    60                 buf_size = MANA_PAGE_ALIGN(roundup_pow_of_two(attr->cqe * COMP_ENTRY_SIZE));
    61                 cq->cqe = buf_size / COMP_ENTRY_SIZE;
    62                 err = mana_ib_create_kernel_queue(mdev, buf_size, GDMA_CQ, &cq->queue);
    63                 if (err) {
    64                         ibdev_dbg(ibdev, "Failed to create kernel queue for create cq, %d\n", err);
    65                         return err;
    66                 }
    67                 doorbell = gc->mana_ib.doorbell;
    68         }
    69 
    70         if (is_rnic_cq) {
    71                 err = mana_ib_gd_create_cq(mdev, cq, doorbell);
    72                 if (err) {
    73                         ibdev_dbg(ibdev, "Failed to create RNIC cq, %d\n", err);
    74                         goto err_destroy_queue;
    75                 }
    76 
    77                 err = mana_ib_install_cq_cb(mdev, cq);
    78                 if (err) {
    79                         ibdev_dbg(ibdev, "Failed to install cq callback, %d\n", err);
    80                         goto err_destroy_rnic_cq;
    81                 }
    82         }
    83 
    84         if (udata) {
    85                 resp.cqid = cq->queue.id;
    86                 err = ib_copy_to_udata(udata, &resp, min(sizeof(resp), udata->outlen));
    87                 if (err) {
    88                         ibdev_dbg(&mdev->ib_dev, "Failed to copy to udata, %d\n", err);
    89                         goto err_remove_cq_cb;
    90                 }
    91         }
    92 
    93         spin_lock_init(&cq->cq_lock);
    94         INIT_LIST_HEAD(&cq->list_send_qp);
    95         INIT_LIST_HEAD(&cq->list_recv_qp);
    96 
    97         return 0;
    98 
    99 err_remove_cq_cb:
    100         mana_ib_remove_cq_cb(mdev, cq);
    101 err_destroy_rnic_cq:
    102         mana_ib_gd_destroy_cq(mdev, cq);
    103 err_destroy_queue:
    104         mana_ib_destroy_queue(mdev, &cq->queue);
    105 
    106         return err;
    107 }

regards,
dan carpenter

Re: [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq

[Leon Romanovsky]

On Wed, Mar 05, 2025 at 10:57:49PM +0300, Dan Carpenter wrote:
> Hello Konstantin Taranov,
> 
> Commit 44b607ad4cdf ("RDMA/mana_ib: implement uapi for creation of
> rnic cq") from Apr 26, 2024 (linux-next), leads to the following
> Smatch static checker warning:
> 
> 	drivers/infiniband/hw/mana/cq.c:48 mana_ib_create_cq()
> 	warn: potential user controlled sizeof overflow 'cq->cqe * 64' 's32min-s32max * 64'
> 
> drivers/infiniband/hw/mana/cq.c
>     8 int mana_ib_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>     9                       struct uverbs_attr_bundle *attrs)
>     10 {
>     11         struct ib_udata *udata = &attrs->driver_udata;
>     12         struct mana_ib_cq *cq = container_of(ibcq, struct mana_ib_cq, ibcq);
>     13         struct mana_ib_create_cq_resp resp = {};
>     14         struct mana_ib_ucontext *mana_ucontext;
>     15         struct ib_device *ibdev = ibcq->device;
>     16         struct mana_ib_create_cq ucmd = {};
>     17         struct mana_ib_dev *mdev;
>     18         struct gdma_context *gc;
>     19         bool is_rnic_cq;
>     20         u32 doorbell;
>     21         u32 buf_size;
>     22         int err;
>     23 
>     24         mdev = container_of(ibdev, struct mana_ib_dev, ib_dev);
>     25         gc = mdev_to_gc(mdev);
>     26 
>     27         cq->comp_vector = attr->comp_vector % ibdev->num_comp_vectors;
>     28         cq->cq_handle = INVALID_MANA_HANDLE;
>     29 
>     30         if (udata) {
>     31                 if (udata->inlen < offsetof(struct mana_ib_create_cq, flags))
>     32                         return -EINVAL;
>     33 
>     34                 err = ib_copy_from_udata(&ucmd, udata, min(sizeof(ucmd), udata->inlen));
>                                                  ^^^^
> ucmd.flags is set by the user here.
> 
>     35                 if (err) {
>     36                         ibdev_dbg(ibdev, "Failed to copy from udata for create cq, %d\n", err);
>     37                         return err;
>     38                 }
>     39 
>     40                 is_rnic_cq = !!(ucmd.flags & MANA_IB_CREATE_RNIC_CQ);
>     41 
>     42                 if (!is_rnic_cq && attr->cqe > mdev->adapter_caps.max_qp_wr) {
>                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> attr->cqe used to be bounds checked every time, but now the user can
> skip setting the flag for bounds checking.
> 
>     43                         ibdev_dbg(ibdev, "CQE %d exceeding limit\n", attr->cqe);
>     44                         return -EINVAL;
>     45                 }
>     46 
>     47                 cq->cqe = attr->cqe;
> --> 48                 err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe * COMP_ENTRY_SIZE,
>                                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This can lead to integer wrapping.
> 
> The call tree is:
> 
> ib_uverbs_create_cq() <- copies cmd.cqe from the user
>   -> create_cq() calls (struct ib_device_ops)->create_cq()
>      -> mana_ib_create_cq()
> 
> I'm not sure if this integer overflow has any negative effects.  I think
> it's probably fine?

It is not nice and worth to be fixed, but technically it looks like size
(cq->cqe * COMP_ENTRY_SIZE) is used to get UMEM memory, so we will allocate
less than driver would like to.

Thanks

RE: [EXTERNAL] Re: [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq

[Konstantin Taranov]

> >     46
> >     47                 cq->cqe = attr->cqe;
> > --> 48                 err = mana_ib_create_queue(mdev, ucmd.buf_addr, cq->cqe *
> COMP_ENTRY_SIZE,
> >
> > ^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This can lead to integer wrapping.
> >
> > The call tree is:
> >
> > ib_uverbs_create_cq() <- copies cmd.cqe from the user
> >   -> create_cq() calls (struct ib_device_ops)->create_cq()
> >      -> mana_ib_create_cq()
> >
> > I'm not sure if this integer overflow has any negative effects.  I
> > think it's probably fine?
> 
> It is not nice and worth to be fixed, but technically it looks like size (cq->cqe *
> COMP_ENTRY_SIZE) is used to get UMEM memory, so we will allocate less
> than driver would like to.
> 
> Thanks

Thanks. I totally agree. I have already prepared a patch.
Once it passes internal tests (in 4 hours), I will send it.

- Konstantin