* [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq
@ 2025-03-05 19:57 Dan Carpenter
2025-03-06 9:01 ` Leon Romanovsky
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-03-05 19:57 UTC (permalink / raw)
To: Konstantin Taranov; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq
2025-03-05 19:57 [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq Dan Carpenter
@ 2025-03-06 9:01 ` Leon Romanovsky
2025-03-06 11:36 ` [EXTERNAL] " Konstantin Taranov
0 siblings, 1 reply; 3+ messages in thread
From: Leon Romanovsky @ 2025-03-06 9:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Konstantin Taranov, linux-rdma
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
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [EXTERNAL] Re: [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq
2025-03-06 9:01 ` Leon Romanovsky
@ 2025-03-06 11:36 ` Konstantin Taranov
0 siblings, 0 replies; 3+ messages in thread
From: Konstantin Taranov @ 2025-03-06 11:36 UTC (permalink / raw)
To: Leon Romanovsky, Dan Carpenter; +Cc: linux-rdma@vger.kernel.org
> > 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-06 11:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-05 19:57 [bug report] RDMA/mana_ib: implement uapi for creation of rnic cq Dan Carpenter
2025-03-06 9:01 ` Leon Romanovsky
2025-03-06 11:36 ` [EXTERNAL] " Konstantin Taranov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox