| Message ID | 5e670b640508e14b1869c3e8e4fb970d78cbe997.1638692171.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/irdma: Fix a potential memory allocation issue in 'irdma_prm_add_pble_mem()' | expand |
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 <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
> 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>
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;
'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(-)