[PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[Christophe JAILLET]

'pchunk->bitmapbuf' is a bitmap. Its size (in number of bits) is stored in
'pchunk->sizeofbitmap'.

When it is allocated, the size (in bytes) is computed by:
   size_in_bits >> 3

There are 2 issues (numbers bellow assume that longs are 64 bits):
   - there is no guarantee here that 'pchunk->bitmapmem.size' is modulo
     BITS_PER_LONG but bitmaps are stored as longs
     (sizeofbitmap=8 bits will only allocate 1 byte, instead of 8 (1 long))

   - the number of bytes is computed with a shift, not a round up, so we
     may allocate less memory than needed
     (sizeofbitmap=65 bits will only allocate 8 bytes (i.e. 1 long), when 2
     longs are needed = 16 bytes)

Fix both issues by using 'bitmap_zalloc()' and remove the useless
'bitmapmem' from 'struct irdma_chunk'.

While at it, remove some useless NULL test before calling
kfree/bitmap_free.

Fixes: 915cc7ac0f8e ("RDMA/irdma: Add miscellaneous utility definitions")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/infiniband/hw/irdma/pble.c  | 6 ++----
 drivers/infiniband/hw/irdma/pble.h  | 1 -
 drivers/infiniband/hw/irdma/utils.c | 9 ++-------
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/pble.c b/drivers/infiniband/hw/irdma/pble.c
index aeeb1c310965..941dd6310161 100644
--- a/drivers/infiniband/hw/irdma/pble.c
+++ b/drivers/infiniband/hw/irdma/pble.c
@@ -25,8 +25,7 @@ void irdma_destroy_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc)
 		list_del(&chunk->list);
 		if (chunk->type == PBLE_SD_PAGED)
 			irdma_pble_free_paged_mem(chunk);
-		if (chunk->bitmapbuf)
-			kfree(chunk->bitmapmem.va);
+		bitmap_free(chunk->bitmapbuf);
 		kfree(chunk->chunkmem.va);
 	}
 }
@@ -299,8 +298,7 @@ add_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc)
 	return 0;
 
 error:
-	if (chunk->bitmapbuf)
-		kfree(chunk->bitmapmem.va);
+	bitmap_free(chunk->bitmapbuf);
 	kfree(chunk->chunkmem.va);
 
 	return ret_code;
diff --git a/drivers/infiniband/hw/irdma/pble.h b/drivers/infiniband/hw/irdma/pble.h
index faf71c99e12e..d0d4f2b77d34 100644
--- a/drivers/infiniband/hw/irdma/pble.h
+++ b/drivers/infiniband/hw/irdma/pble.h
@@ -78,7 +78,6 @@ struct irdma_chunk {
 	u32 pg_cnt;
 	enum irdma_alloc_type type;
 	struct irdma_sc_dev *dev;
-	struct irdma_virt_mem bitmapmem;
 	struct irdma_virt_mem chunkmem;
 };
 
diff --git a/drivers/infiniband/hw/irdma/utils.c b/drivers/infiniband/hw/irdma/utils.c
index 8b42c43fc14f..981107b40c90 100644
--- a/drivers/infiniband/hw/irdma/utils.c
+++ b/drivers/infiniband/hw/irdma/utils.c
@@ -2239,15 +2239,10 @@ enum irdma_status_code irdma_prm_add_pble_mem(struct irdma_pble_prm *pprm,
 
 	sizeofbitmap = (u64)pchunk->size >> pprm->pble_shift;
 
-	pchunk->bitmapmem.size = sizeofbitmap >> 3;
-	pchunk->bitmapmem.va = kzalloc(pchunk->bitmapmem.size, GFP_KERNEL);
-
-	if (!pchunk->bitmapmem.va)
+	pchunk->bitmapbuf = bitmap_zalloc(sizeofbitmap, GFP_KERNEL);
+	if (!pchunk->bitmapbuf)
 		return IRDMA_ERR_NO_MEMORY;
 
-	pchunk->bitmapbuf = pchunk->bitmapmem.va;
-	bitmap_zero(pchunk->bitmapbuf, sizeofbitmap);
-
 	pchunk->sizeofbitmap = sizeofbitmap;
 	/* each pble is 8 bytes hence shift by 3 */
 	pprm->total_pble_alloc += pchunk->size >> 3;
-- 
2.30.2

Re: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[Dan Carpenter]

On Sun, Dec 05, 2021 at 09:17:24AM +0100, Christophe JAILLET wrote:
> @@ -299,8 +298,7 @@ add_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc)
>  	return 0;
>  
>  error:
> -	if (chunk->bitmapbuf)
> -		kfree(chunk->bitmapmem.va);
> +	bitmap_free(chunk->bitmapbuf);
>  	kfree(chunk->chunkmem.va);

Thanks for removing the "chunk->bitmapbuf = chunk->bitmapmem.va;" stuff.
It was really confusing.  The kfree(chunk->chunkmem.va) is equivalent to
kfree(chunk).  A good rule of thumb is that when you have one error:
label to free everything then it's normally going to be buggy.

drivers/infiniband/hw/irdma/pble.c
   281          pble_rsrc->next_fpm_addr += chunk->size;
   282          ibdev_dbg(to_ibdev(dev),
   283                    "PBLE: next_fpm_addr = %llx chunk_size[%llu] = 0x%llx\n",
   284                    pble_rsrc->next_fpm_addr, chunk->size, chunk->size);
   285          pble_rsrc->unallocated_pble -= (u32)(chunk->size >> 3);
   286          list_add(&chunk->list, &pble_rsrc->pinfo.clist);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"chunk" added to the "->pinfo.clist" list.

   287          sd_reg_val = (sd_entry_type == IRDMA_SD_TYPE_PAGED) ?
   288                               sd_entry->u.pd_table.pd_page_addr.pa :
   289                               sd_entry->u.bp.addr.pa;
   290  
   291          if (!sd_entry->valid) {
   292                  ret_code = irdma_hmc_sd_one(dev, hmc_info->hmc_fn_id, sd_reg_val,
   293                                              idx->sd_idx, sd_entry->entry_type, true);
   294                  if (ret_code)
   295                          goto error;
                                ^^^^^^^^^^^

   296          }
   297  
   298          sd_entry->valid = true;
   299          return 0;
   300  
   301  error:
   302          if (chunk->bitmapbuf)
   303                  kfree(chunk->bitmapmem.va);
   304          kfree(chunk->chunkmem.va);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^
kfree(chunk) will lead to a use after free because it's still on the
list.

   305  
   306          return ret_code;
   307  }

regards,
dan carpenter

RE: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[Saleem, Shiraz]

> Subject: Re: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in
> 'irdma_prm_add_pble_mem()'
> 
> On Sun, Dec 05, 2021 at 09:17:24AM +0100, Christophe JAILLET wrote:
> > @@ -299,8 +298,7 @@ add_pble_prm(struct irdma_hmc_pble_rsrc *pble_rsrc)
> >  	return 0;
> >
> >  error:
> > -	if (chunk->bitmapbuf)
> > -		kfree(chunk->bitmapmem.va);
> > +	bitmap_free(chunk->bitmapbuf);
> >  	kfree(chunk->chunkmem.va);
> 
> Thanks for removing the "chunk->bitmapbuf = chunk->bitmapmem.va;" stuff.
> It was really confusing.  The kfree(chunk->chunkmem.va) is equivalent to
> kfree(chunk).  A good rule of thumb is that when you have one error:
> label to free everything then it's normally going to be buggy.
> 
> drivers/infiniband/hw/irdma/pble.c
>    281          pble_rsrc->next_fpm_addr += chunk->size;
>    282          ibdev_dbg(to_ibdev(dev),
>    283                    "PBLE: next_fpm_addr = %llx chunk_size[%llu] = 0x%llx\n",
>    284                    pble_rsrc->next_fpm_addr, chunk->size, chunk->size);
>    285          pble_rsrc->unallocated_pble -= (u32)(chunk->size >> 3);
>    286          list_add(&chunk->list, &pble_rsrc->pinfo.clist);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "chunk" added to the "->pinfo.clist" list.
> 
>    287          sd_reg_val = (sd_entry_type == IRDMA_SD_TYPE_PAGED) ?
>    288                               sd_entry->u.pd_table.pd_page_addr.pa :
>    289                               sd_entry->u.bp.addr.pa;
>    290
>    291          if (!sd_entry->valid) {
>    292                  ret_code = irdma_hmc_sd_one(dev, hmc_info->hmc_fn_id,
> sd_reg_val,
>    293                                              idx->sd_idx, sd_entry->entry_type, true);
>    294                  if (ret_code)
>    295                          goto error;
>                                 ^^^^^^^^^^^
> 
>    296          }
>    297
>    298          sd_entry->valid = true;
>    299          return 0;
>    300
>    301  error:
>    302          if (chunk->bitmapbuf)
>    303                  kfree(chunk->bitmapmem.va);
>    304          kfree(chunk->chunkmem.va);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
> kfree(chunk) will lead to a use after free because it's still on the list.
> 

Ugh! Yes, this is a bug. I will send a separate fix out shortly for this one.

Shiraz

Re: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[Jason Gunthorpe]

On Sun, Dec 05, 2021 at 09:17:24AM +0100, Christophe JAILLET wrote:
> 'pchunk->bitmapbuf' is a bitmap. Its size (in number of bits) is stored in
> 'pchunk->sizeofbitmap'.
> 
> When it is allocated, the size (in bytes) is computed by:
>    size_in_bits >> 3
> 
> There are 2 issues (numbers bellow assume that longs are 64 bits):
>    - there is no guarantee here that 'pchunk->bitmapmem.size' is modulo
>      BITS_PER_LONG but bitmaps are stored as longs
>      (sizeofbitmap=8 bits will only allocate 1 byte, instead of 8 (1 long))
> 
>    - the number of bytes is computed with a shift, not a round up, so we
>      may allocate less memory than needed
>      (sizeofbitmap=65 bits will only allocate 8 bytes (i.e. 1 long), when 2
>      longs are needed = 16 bytes)
> 
> Fix both issues by using 'bitmap_zalloc()' and remove the useless
> 'bitmapmem' from 'struct irdma_chunk'.
> 
> While at it, remove some useless NULL test before calling
> kfree/bitmap_free.
> 
> Fixes: 915cc7ac0f8e ("RDMA/irdma: Add miscellaneous utility definitions")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/infiniband/hw/irdma/pble.c  | 6 ++----
>  drivers/infiniband/hw/irdma/pble.h  | 1 -
>  drivers/infiniband/hw/irdma/utils.c | 9 ++-------
>  3 files changed, 4 insertions(+), 12 deletions(-)

Applied to for-rc, thanks

Jason

RE: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()'

[Saleem, Shiraz]

> Subject: [PATCH] RDMA/irdma: Fix a potential memory allocation issue in
> 'irdma_prm_add_pble_mem()'
> 
> 'pchunk->bitmapbuf' is a bitmap. Its size (in number of bits) is stored in 'pchunk-
> >sizeofbitmap'.
> 
> When it is allocated, the size (in bytes) is computed by:
>    size_in_bits >> 3
> 
> There are 2 issues (numbers bellow assume that longs are 64 bits):
>    - there is no guarantee here that 'pchunk->bitmapmem.size' is modulo
>      BITS_PER_LONG but bitmaps are stored as longs
>      (sizeofbitmap=8 bits will only allocate 1 byte, instead of 8 (1 long))
> 
>    - the number of bytes is computed with a shift, not a round up, so we
>      may allocate less memory than needed
>      (sizeofbitmap=65 bits will only allocate 8 bytes (i.e. 1 long), when 2
>      longs are needed = 16 bytes)

Since sizeofbitmap is always a multiple of 64 (pchunk->size is a multiple of 4K block size, and pprm->pble_shift = 6),
I am not sure we will hit these issues today.

> 
> Fix both issues by using 'bitmap_zalloc()' and remove the useless 'bitmapmem'
> from 'struct irdma_chunk'.
> 
> While at it, remove some useless NULL test before calling kfree/bitmap_free.

Yes nonetheless the patch is good. And we should be using the bitmap_zalloc/free API's rather than open-coding it.

Reviewed-by: Shiraz Saleem <shiraz.saleem@intel.com>