diff mbox series

[for-next,3/5] RDMA/efa: Validate EQ array out of bounds reach

Message ID 20240624160918.27060-4-mrgolin@amazon.com (mailing list archive)
State Changes Requested
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/efa: Cleanups and minor improvements | expand

Commit Message

Margolin, Michael June 24, 2024, 4:09 p.m. UTC 
From: Yonatan Nachum <ynachum@amazon.com>

When creating a new CQ with interrupts enabled, the caller needs to
specify an EQ index to which the interrupts will be sent on, we don't
validate the requested index in the EQ array.
Validate out of bound reach of the EQ array and return an error.

This is not a bug because IB core validates the requested EQ number when
creating a CQ.

Reviewed-by: Firas Jahjah <firasj@amazon.com>
Signed-off-by: Yonatan Nachum <ynachum@amazon.com>
Signed-off-by: Michael Margolin <mrgolin@amazon.com>
---
 drivers/infiniband/hw/efa/efa_verbs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Gal Pressman June 25, 2024, 6:33 a.m. UTC | #1 
On 24/06/2024 19:09, Michael Margolin wrote:
> From: Yonatan Nachum <ynachum@amazon.com>
> 
> When creating a new CQ with interrupts enabled, the caller needs to
> specify an EQ index to which the interrupts will be sent on, we don't
> validate the requested index in the EQ array.
> Validate out of bound reach of the EQ array and return an error.
> 
> This is not a bug because IB core validates the requested EQ number when
> creating a CQ.

Then why is this patch needed?

> 
> Reviewed-by: Firas Jahjah <firasj@amazon.com>
> Signed-off-by: Yonatan Nachum <ynachum@amazon.com>
> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
Margolin, Michael June 25, 2024, 11:28 a.m. UTC | #2 
On 6/25/2024 9:33 AM, Gal Pressman wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 24/06/2024 19:09, Michael Margolin wrote:
>> From: Yonatan Nachum <ynachum@amazon.com>
>>
>> When creating a new CQ with interrupts enabled, the caller needs to
>> specify an EQ index to which the interrupts will be sent on, we don't
>> validate the requested index in the EQ array.
>> Validate out of bound reach of the EQ array and return an error.
>>
>> This is not a bug because IB core validates the requested EQ number when
>> creating a CQ.
> Then why is this patch needed?

Mainly as best practice of validating the index right before array 
access to avoid hard to catch bugs.

Michael
Leon Romanovsky June 25, 2024, 2:16 p.m. UTC | #3 
On Tue, Jun 25, 2024 at 02:28:24PM +0300, Margolin, Michael wrote:
> 
> On 6/25/2024 9:33 AM, Gal Pressman wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On 24/06/2024 19:09, Michael Margolin wrote:
> > > From: Yonatan Nachum <ynachum@amazon.com>
> > > 
> > > When creating a new CQ with interrupts enabled, the caller needs to
> > > specify an EQ index to which the interrupts will be sent on, we don't
> > > validate the requested index in the EQ array.
> > > Validate out of bound reach of the EQ array and return an error.
> > > 
> > > This is not a bug because IB core validates the requested EQ number when
> > > creating a CQ.
> > Then why is this patch needed?
> 
> Mainly as best practice of validating the index right before array access to
> avoid hard to catch bugs.

If you know about the bug, please fix it. There is no need to add code
"to protect" low level from incorrect upper level.

Thanks

> 
> Michael
> 
>
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index cd1f735d08a7..9c3e476e3f9c 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -1046,7 +1046,7 @@  int efa_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 
 static struct efa_eq *efa_vec2eq(struct efa_dev *dev, int vec)
 {
-	return &dev->eqs[vec];
+	return vec < dev->neqs ? &dev->eqs[vec] : NULL;
 }
 
 static int cq_mmap_entries_setup(struct efa_dev *dev, struct efa_cq *cq,
@@ -1173,6 +1173,11 @@  int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	params.set_src_addr = set_src_addr;
 	if (cmd.flags & EFA_CREATE_CQ_WITH_COMPLETION_CHANNEL) {
 		cq->eq = efa_vec2eq(dev, attr->comp_vector);
+		if (!cq->eq) {
+			ibdev_dbg(ibdev, "Invalid EQ requested[%u]\n", attr->comp_vector);
+			err = -EINVAL;
+			goto err_free_mapped;
+		}
 		params.eqn = cq->eq->eeq.eqn;
 		params.interrupt_mode_enabled = true;
 	}