All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH rdma-next 0/3] irdma coverity fixes
@ 2021-06-22 17:52 Tatyana Nikolova
  2021-06-22 17:52 ` [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object Tatyana Nikolova
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Tatyana Nikolova @ 2021-06-22 17:52 UTC (permalink / raw)
  To: jgg, dledford; +Cc: linux-rdma, shiraz.saleem, mustafa.ismail, Tatyana Nikolova

This is a short series of coverity fixes for irdma.

Shiraz Saleem (3):
  RDMA/irdma: Check contents of user-space irdma_mem_reg_req object
  RDMA/irdma: Check return value from ib_umem_find_best_pgsz
  RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles

 drivers/infiniband/hw/irdma/pble.h  |  2 +-
 drivers/infiniband/hw/irdma/utils.c |  4 ++--
 drivers/infiniband/hw/irdma/verbs.c | 26 +++++++++++++++++++++-----
 3 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object
  2021-06-22 17:52 [PATCH rdma-next 0/3] irdma coverity fixes Tatyana Nikolova
@ 2021-06-22 17:52 ` Tatyana Nikolova
  2021-06-22 17:58   ` Jason Gunthorpe
  2021-06-22 17:52 ` [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz Tatyana Nikolova
  2021-06-22 17:52 ` [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles Tatyana Nikolova
  2 siblings, 1 reply; 9+ messages in thread
From: Tatyana Nikolova @ 2021-06-22 17:52 UTC (permalink / raw)
  To: jgg, dledford
  Cc: linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot,
	Tatyana Nikolova

From: Shiraz Saleem <shiraz.saleem@intel.com>

The contents of user-space req object is used in array indexing
in irdma_handle_q_mem without checking for valid values.

Guard against bad input on each of these req object pages by
limiting them to number of pages that make up the region.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1505160 ("TAINTED_SCALAR")
Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
---
 drivers/infiniband/hw/irdma/verbs.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index e8b170f0d997..8bd31656a83a 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -2360,10 +2360,8 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
 	u64 *arr = iwmr->pgaddrmem;
 	u32 pg_size;
 	int err = 0;
-	int total;
 	bool ret = true;
 
-	total = req->sq_pages + req->rq_pages + req->cq_pages;
 	pg_size = iwmr->page_size;
 	err = irdma_setup_pbles(iwdev->rf, iwmr, use_pbles);
 	if (err)
@@ -2381,7 +2379,7 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
 	switch (iwmr->type) {
 	case IRDMA_MEMREG_TYPE_QP:
 		hmc_p = &qpmr->sq_pbl;
-		qpmr->shadow = (dma_addr_t)arr[total];
+		qpmr->shadow = (dma_addr_t)arr[req->sq_pages + req->rq_pages];
 
 		if (use_pbles) {
 			ret = irdma_check_mem_contiguous(arr, req->sq_pages,
@@ -2406,7 +2404,7 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
 		hmc_p = &cqmr->cq_pbl;
 
 		if (!cqmr->split)
-			cqmr->shadow = (dma_addr_t)arr[total];
+			cqmr->shadow = (dma_addr_t)arr[req->cq_pages];
 
 		if (use_pbles)
 			ret = irdma_check_mem_contiguous(arr, req->cq_pages,
@@ -2748,6 +2746,7 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
 	struct ib_umem *region;
 	struct irdma_mem_reg_req req;
 	u32 stag = 0;
+	u8 shadow_pgcnt = 1;
 	bool use_pbles = false;
 	unsigned long flags;
 	int err = -EINVAL;
@@ -2795,6 +2794,10 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
 
 	switch (req.reg_type) {
 	case IRDMA_MEMREG_TYPE_QP:
+		if (req.sq_pages + req.rq_pages + shadow_pgcnt > iwmr->page_cnt) {
+			err = -EINVAL;
+			goto error;
+		}
 		use_pbles = ((req.sq_pages + req.rq_pages) > 2);
 		err = irdma_handle_q_mem(iwdev, &req, iwpbl, use_pbles);
 		if (err)
@@ -2808,6 +2811,13 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
 		spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
 		break;
 	case IRDMA_MEMREG_TYPE_CQ:
+		if (iwdev->rf->sc_dev.hw_attrs.uk_attrs.feature_flags & IRDMA_FEATURE_CQ_RESIZE)
+			shadow_pgcnt = 0;
+		if (req.cq_pages + shadow_pgcnt > iwmr->page_cnt) {
+			err = -EINVAL;
+			goto error;
+		}
+
 		use_pbles = (req.cq_pages > 1);
 		err = irdma_handle_q_mem(iwdev, &req, iwpbl, use_pbles);
 		if (err)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz
  2021-06-22 17:52 [PATCH rdma-next 0/3] irdma coverity fixes Tatyana Nikolova
  2021-06-22 17:52 ` [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object Tatyana Nikolova
@ 2021-06-22 17:52 ` Tatyana Nikolova
  2021-06-22 18:28   ` Jason Gunthorpe
  2021-06-22 17:52 ` [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles Tatyana Nikolova
  2 siblings, 1 reply; 9+ messages in thread
From: Tatyana Nikolova @ 2021-06-22 17:52 UTC (permalink / raw)
  To: jgg, dledford
  Cc: linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot,
	Tatyana Nikolova

From: Shiraz Saleem <shiraz.saleem@intel.com>

iwmr->page_size stores the return from ib_umem_find_best_pgsz
and maybe zero when used in ib_umem_num_dma_blocks thus causing
a divide by zero error.

Fix this by erroring out of irdma_reg_user when 0 is returned
from ib_umem_find_best_pgsz.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1505149 ("Integer handling issues")
Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
---
 drivers/infiniband/hw/irdma/verbs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
index 8bd31656a83a..2c4f67fac360 100644
--- a/drivers/infiniband/hw/irdma/verbs.c
+++ b/drivers/infiniband/hw/irdma/verbs.c
@@ -2782,10 +2782,16 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
 	iwmr->ibmr.iova = virt;
 	iwmr->page_size = PAGE_SIZE;
 
-	if (req.reg_type == IRDMA_MEMREG_TYPE_MEM)
+	if (req.reg_type == IRDMA_MEMREG_TYPE_MEM) {
 		iwmr->page_size = ib_umem_find_best_pgsz(region,
 							 SZ_4K | SZ_2M | SZ_1G,
 							 virt);
+		if (unlikely(!iwmr->page_size)) {
+			kfree(iwmr);
+			ib_umem_release(region);
+			return ERR_PTR(-EOPNOTSUPP);
+		}
+	}
 	iwmr->len = region->length;
 	iwpbl->user_base = virt;
 	palloc = &iwpbl->pble_alloc;
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles
  2021-06-22 17:52 [PATCH rdma-next 0/3] irdma coverity fixes Tatyana Nikolova
  2021-06-22 17:52 ` [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object Tatyana Nikolova
  2021-06-22 17:52 ` [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz Tatyana Nikolova
@ 2021-06-22 17:52 ` Tatyana Nikolova
  2021-06-22 18:07   ` Jason Gunthorpe
  2 siblings, 1 reply; 9+ messages in thread
From: Tatyana Nikolova @ 2021-06-22 17:52 UTC (permalink / raw)
  To: jgg, dledford
  Cc: linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot,
	Tatyana Nikolova

From: Shiraz Saleem <shiraz.saleem@intel.com>

Coverity reports a signed 32-bit overflow on "1 << pprm->pble_shift" when
used expression to compute bits_needed that expects 64bit, unsigned.

Fix this by using the 1ULL in the left shift operator and convert
mem_size to u64.

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1505157 ("Integer handling issues")
Fixes: 915cc7ac0f8e ("RDMA/irdma: Add miscellaneous utility definitions")
Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
---
 drivers/infiniband/hw/irdma/pble.h  | 2 +-
 drivers/infiniband/hw/irdma/utils.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/pble.h b/drivers/infiniband/hw/irdma/pble.h
index e4e635dc4fd9..e1b3b8118a2c 100644
--- a/drivers/infiniband/hw/irdma/pble.h
+++ b/drivers/infiniband/hw/irdma/pble.h
@@ -121,7 +121,7 @@ enum irdma_status_code irdma_prm_add_pble_mem(struct irdma_pble_prm *pprm,
 					      struct irdma_chunk *pchunk);
 enum irdma_status_code
 irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
-		    struct irdma_pble_chunkinfo *chunkinfo, u32 mem_size,
+		    struct irdma_pble_chunkinfo *chunkinfo, u64 mem_size,
 		    u64 **vaddr, u64 *fpm_addr);
 void irdma_prm_return_pbles(struct irdma_pble_prm *pprm,
 			    struct irdma_pble_chunkinfo *chunkinfo);
diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
index ea1df5918c11..e50b6f89b37e 100644
--- a/drivers/infiniband/hw/irdma/utils.c
+++ b/drivers/infiniband/hw/irdma/utils.c
@@ -2314,7 +2314,7 @@ enum irdma_status_code irdma_prm_add_pble_mem(struct irdma_pble_prm *pprm,
  */
 enum irdma_status_code
 irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
-		    struct irdma_pble_chunkinfo *chunkinfo, u32 mem_size,
+		    struct irdma_pble_chunkinfo *chunkinfo, u64 mem_size,
 		    u64 **vaddr, u64 *fpm_addr)
 {
 	u64 bits_needed;
@@ -2326,7 +2326,7 @@ irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
 	*vaddr = NULL;
 	*fpm_addr = 0;
 
-	bits_needed = (mem_size + (1 << pprm->pble_shift) - 1) >> pprm->pble_shift;
+	bits_needed = (mem_size + BIT_ULL(pprm->pble_shift) - 1) >> pprm->pble_shift;
 
 	spin_lock_irqsave(&pprm->prm_lock, flags);
 	while (chunk_entry != &pprm->clist) {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object
  2021-06-22 17:52 ` [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object Tatyana Nikolova
@ 2021-06-22 17:58   ` Jason Gunthorpe
  2021-06-22 21:56     ` Nikolova, Tatyana E
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2021-06-22 17:58 UTC (permalink / raw)
  To: Tatyana Nikolova
  Cc: dledford, linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot

On Tue, Jun 22, 2021 at 12:52:30PM -0500, Tatyana Nikolova wrote:
> From: Shiraz Saleem <shiraz.saleem@intel.com>
> 
> The contents of user-space req object is used in array indexing
> in irdma_handle_q_mem without checking for valid values.
> 
> Guard against bad input on each of these req object pages by
> limiting them to number of pages that make up the region.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1505160 ("TAINTED_SCALAR")
> Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
>  drivers/infiniband/hw/irdma/verbs.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index e8b170f0d997..8bd31656a83a 100644
> +++ b/drivers/infiniband/hw/irdma/verbs.c
> @@ -2360,10 +2360,8 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
>  	u64 *arr = iwmr->pgaddrmem;
>  	u32 pg_size;
>  	int err = 0;
> -	int total;
>  	bool ret = true;
>  
> -	total = req->sq_pages + req->rq_pages + req->cq_pages;
>  	pg_size = iwmr->page_size;
>  	err = irdma_setup_pbles(iwdev->rf, iwmr, use_pbles);
>  	if (err)
> @@ -2381,7 +2379,7 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
>  	switch (iwmr->type) {
>  	case IRDMA_MEMREG_TYPE_QP:
>  		hmc_p = &qpmr->sq_pbl;
> -		qpmr->shadow = (dma_addr_t)arr[total];
> +		qpmr->shadow = (dma_addr_t)arr[req->sq_pages + req->rq_pages];
>  
>  		if (use_pbles) {
>  			ret = irdma_check_mem_contiguous(arr, req->sq_pages,
> @@ -2406,7 +2404,7 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
>  		hmc_p = &cqmr->cq_pbl;
>  
>  		if (!cqmr->split)
> -			cqmr->shadow = (dma_addr_t)arr[total];
> +			cqmr->shadow = (dma_addr_t)arr[req->cq_pages];
>  
>  		if (use_pbles)
>  			ret = irdma_check_mem_contiguous(arr, req->cq_pages,
> @@ -2748,6 +2746,7 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
>  	struct ib_umem *region;
>  	struct irdma_mem_reg_req req;
>  	u32 stag = 0;
> +	u8 shadow_pgcnt = 1;
>  	bool use_pbles = false;
>  	unsigned long flags;
>  	int err = -EINVAL;
> @@ -2795,6 +2794,10 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64 start, u64 len,
>  
>  	switch (req.reg_type) {
>  	case IRDMA_MEMREG_TYPE_QP:
> +		if (req.sq_pages + req.rq_pages + shadow_pgcnt > iwmr->page_cnt) {

Math on values from userspace should use the check overflow helpers or
otherwise be designed to be overflow safe

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles
  2021-06-22 17:52 ` [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles Tatyana Nikolova
@ 2021-06-22 18:07   ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2021-06-22 18:07 UTC (permalink / raw)
  To: Tatyana Nikolova
  Cc: dledford, linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot

On Tue, Jun 22, 2021 at 12:52:32PM -0500, Tatyana Nikolova wrote:
> From: Shiraz Saleem <shiraz.saleem@intel.com>
> 
> Coverity reports a signed 32-bit overflow on "1 << pprm->pble_shift" when
> used expression to compute bits_needed that expects 64bit, unsigned.
> 
> Fix this by using the 1ULL in the left shift operator and convert
> mem_size to u64.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1505157 ("Integer handling issues")
> Fixes: 915cc7ac0f8e ("RDMA/irdma: Add miscellaneous utility definitions")
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
>  drivers/infiniband/hw/irdma/pble.h  | 2 +-
>  drivers/infiniband/hw/irdma/utils.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/irdma/pble.h b/drivers/infiniband/hw/irdma/pble.h
> index e4e635dc4fd9..e1b3b8118a2c 100644
> +++ b/drivers/infiniband/hw/irdma/pble.h
> @@ -121,7 +121,7 @@ enum irdma_status_code irdma_prm_add_pble_mem(struct irdma_pble_prm *pprm,
>  					      struct irdma_chunk *pchunk);
>  enum irdma_status_code
>  irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
> -		    struct irdma_pble_chunkinfo *chunkinfo, u32 mem_size,
> +		    struct irdma_pble_chunkinfo *chunkinfo, u64 mem_size,
>  		    u64 **vaddr, u64 *fpm_addr);
>  void irdma_prm_return_pbles(struct irdma_pble_prm *pprm,
>  			    struct irdma_pble_chunkinfo *chunkinfo);
> diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
> index ea1df5918c11..e50b6f89b37e 100644
> +++ b/drivers/infiniband/hw/irdma/utils.c
> @@ -2314,7 +2314,7 @@ enum irdma_status_code irdma_prm_add_pble_mem(struct irdma_pble_prm *pprm,
>   */
>  enum irdma_status_code
>  irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
> -		    struct irdma_pble_chunkinfo *chunkinfo, u32 mem_size,
> +		    struct irdma_pble_chunkinfo *chunkinfo, u64 mem_size,
>  		    u64 **vaddr, u64 *fpm_addr)
>  {
>  	u64 bits_needed;
> @@ -2326,7 +2326,7 @@ irdma_prm_get_pbles(struct irdma_pble_prm *pprm,
>  	*vaddr = NULL;
>  	*fpm_addr = 0;
>  
> -	bits_needed = (mem_size + (1 << pprm->pble_shift) - 1) >> pprm->pble_shift;
> +	bits_needed = (mem_size + BIT_ULL(pprm->pble_shift) - 1) >> pprm->pble_shift;

Isn't this just DIV_ROUND_UP_ULL() ?

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz
  2021-06-22 17:52 ` [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz Tatyana Nikolova
@ 2021-06-22 18:28   ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2021-06-22 18:28 UTC (permalink / raw)
  To: Tatyana Nikolova
  Cc: dledford, linux-rdma, shiraz.saleem, mustafa.ismail, coverity-bot

On Tue, Jun 22, 2021 at 12:52:31PM -0500, Tatyana Nikolova wrote:
> From: Shiraz Saleem <shiraz.saleem@intel.com>
> 
> iwmr->page_size stores the return from ib_umem_find_best_pgsz
> and maybe zero when used in ib_umem_num_dma_blocks thus causing
> a divide by zero error.
> 
> Fix this by erroring out of irdma_reg_user when 0 is returned
> from ib_umem_find_best_pgsz.
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1505149 ("Integer handling issues")
> Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb APIs")
> Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
> ---
>  drivers/infiniband/hw/irdma/verbs.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

This patch applied to for-next, thanks

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object
  2021-06-22 17:58   ` Jason Gunthorpe
@ 2021-06-22 21:56     ` Nikolova, Tatyana E
  2021-06-22 23:33       ` Jason Gunthorpe
  0 siblings, 1 reply; 9+ messages in thread
From: Nikolova, Tatyana E @ 2021-06-22 21:56 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: dledford, linux-rdma, Saleem, Shiraz, Ismail, Mustafa, coverity-bot



> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, June 22, 2021 12:59 PM
> To: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Saleem, Shiraz
> <shiraz.saleem@intel.com>; Ismail, Mustafa <mustafa.ismail@intel.com>;
> coverity-bot <keescook+coverity-bot@chromium.org>
> Subject: Re: [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-
> space irdma_mem_reg_req object
> 
> On Tue, Jun 22, 2021 at 12:52:30PM -0500, Tatyana Nikolova wrote:
> > From: Shiraz Saleem <shiraz.saleem@intel.com>
> >
> > The contents of user-space req object is used in array indexing in
> > irdma_handle_q_mem without checking for valid values.
> >
> > Guard against bad input on each of these req object pages by limiting
> > them to number of pages that make up the region.
> >
> > Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> > Addresses-Coverity-ID: 1505160 ("TAINTED_SCALAR")
> > Fixes: b48c24c2d710 ("RDMA/irdma: Implement device supported verb
> > APIs")
> > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > Signed-off-by: Tatyana Nikolova <tatyana.e.nikolova@intel.com>
> > drivers/infiniband/hw/irdma/verbs.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/irdma/verbs.c
> > b/drivers/infiniband/hw/irdma/verbs.c
> > index e8b170f0d997..8bd31656a83a 100644
> > +++ b/drivers/infiniband/hw/irdma/verbs.c
> > @@ -2360,10 +2360,8 @@ static int irdma_handle_q_mem(struct
> irdma_device *iwdev,
> >  	u64 *arr = iwmr->pgaddrmem;
> >  	u32 pg_size;
> >  	int err = 0;
> > -	int total;
> >  	bool ret = true;
> >
> > -	total = req->sq_pages + req->rq_pages + req->cq_pages;
> >  	pg_size = iwmr->page_size;
> >  	err = irdma_setup_pbles(iwdev->rf, iwmr, use_pbles);
> >  	if (err)
> > @@ -2381,7 +2379,7 @@ static int irdma_handle_q_mem(struct
> irdma_device *iwdev,
> >  	switch (iwmr->type) {
> >  	case IRDMA_MEMREG_TYPE_QP:
> >  		hmc_p = &qpmr->sq_pbl;
> > -		qpmr->shadow = (dma_addr_t)arr[total];
> > +		qpmr->shadow = (dma_addr_t)arr[req->sq_pages + req-
> >rq_pages];
> >
> >  		if (use_pbles) {
> >  			ret = irdma_check_mem_contiguous(arr, req-
> >sq_pages, @@ -2406,7
> > +2404,7 @@ static int irdma_handle_q_mem(struct irdma_device *iwdev,
> >  		hmc_p = &cqmr->cq_pbl;
> >
> >  		if (!cqmr->split)
> > -			cqmr->shadow = (dma_addr_t)arr[total];
> > +			cqmr->shadow = (dma_addr_t)arr[req->cq_pages];
> >
> >  		if (use_pbles)
> >  			ret = irdma_check_mem_contiguous(arr, req-
> >cq_pages, @@ -2748,6
> > +2746,7 @@ static struct ib_mr *irdma_reg_user_mr(struct ib_pd *pd, u64
> start, u64 len,
> >  	struct ib_umem *region;
> >  	struct irdma_mem_reg_req req;
> >  	u32 stag = 0;
> > +	u8 shadow_pgcnt = 1;
> >  	bool use_pbles = false;
> >  	unsigned long flags;
> >  	int err = -EINVAL;
> > @@ -2795,6 +2794,10 @@ static struct ib_mr *irdma_reg_user_mr(struct
> > ib_pd *pd, u64 start, u64 len,
> >
> >  	switch (req.reg_type) {
> >  	case IRDMA_MEMREG_TYPE_QP:
> > +		if (req.sq_pages + req.rq_pages + shadow_pgcnt > iwmr-
> >page_cnt) {
> 
> Math on values from userspace should use the check overflow helpers or
> otherwise be designed to be overflow safe
> 
Hi Jason,

The mem_reg_req fields sq_pages and rq_pages are u16 and the variable shadow_pgcnt is u8. They should be promoted to u32 when compared with iwmr->page_cnt which is u32. Isn't this overflow safe? 

Is the issue you are mentioning about this line:
> > +		qpmr->shadow = (dma_addr_t)arr[req->sq_pages + req-
> >rq_pages];

Thank you,
Tatyana

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object
  2021-06-22 21:56     ` Nikolova, Tatyana E
@ 2021-06-22 23:33       ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2021-06-22 23:33 UTC (permalink / raw)
  To: Nikolova, Tatyana E
  Cc: dledford, linux-rdma, Saleem, Shiraz, Ismail, Mustafa, coverity-bot

On Tue, Jun 22, 2021 at 09:56:42PM +0000, Nikolova, Tatyana E wrote:
> > >  	switch (req.reg_type) {
> > >  	case IRDMA_MEMREG_TYPE_QP:
> > > +		if (req.sq_pages + req.rq_pages + shadow_pgcnt > iwmr-
> > >page_cnt) {
> > 
> > Math on values from userspace should use the check overflow helpers or
> > otherwise be designed to be overflow safe
> 
> The mem_reg_req fields sq_pages and rq_pages are u16 and the
> variable shadow_pgcnt is u8. They should be promoted to u32 when
> compared with iwmr->page_cnt which is u32. Isn't this overflow safe?

I didn't check the sizes carefully, and I'm always nervous about
relying on implicit promotion for security properties as it is so
subtle and easy to get screwed up during maintenance

> Is the issue you are mentioning about this line:
> > > +		qpmr->shadow = (dma_addr_t)arr[req->sq_pages + req->rq_pages];

I assume this is safe because of the if above?

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-22 23:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 17:52 [PATCH rdma-next 0/3] irdma coverity fixes Tatyana Nikolova
2021-06-22 17:52 ` [PATCH rdma-next 1/3] RDMA/irdma: Check contents of user-space irdma_mem_reg_req object Tatyana Nikolova
2021-06-22 17:58   ` Jason Gunthorpe
2021-06-22 21:56     ` Nikolova, Tatyana E
2021-06-22 23:33       ` Jason Gunthorpe
2021-06-22 17:52 ` [PATCH rdma-next 2/3] RDMA/irdma: Check return value from ib_umem_find_best_pgsz Tatyana Nikolova
2021-06-22 18:28   ` Jason Gunthorpe
2021-06-22 17:52 ` [PATCH rdma-next 3/3] RDMA/irdma: Fix potential overflow expression in irdma_prm_get_pbles Tatyana Nikolova
2021-06-22 18:07   ` Jason Gunthorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.