'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 <[email protected]>
---
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
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
> 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
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 <[email protected]>
> ---
> 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
> 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 <[email protected]>