[PATCH] RDMA/bnxt_re: fix a size calculation

[PATCH] RDMA/bnxt_re: fix a size calculation

[Dan Carpenter]

This is from static analysis not from testing.  Depending on the value
of rcfw->cmdq_depth, then this might not cause an issue at runtime.

The BITS_TO_LONGS() macro tells us how many longs it take to hold a
bitmap.  In other words, it divides by the number if bits per long and
rounds up.  Then we want to take that number and multiple by
sizeof(long) to get the number of bytes to allocate.

The code here does the multiplication first so the rounding up is done
in the wrong place.  So imagine we want to allocate 1 bit, then
"(1 * 8) / 64 = 1" when we round up.  But it should be
"(1 / 64) * 8 = 8".  In other words, because of the rounding difference
we might allocate up to "sizeof(long) - 1" bytes fewer than intended.

Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index 326805461265..742346ea5b0d 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -684,8 +684,7 @@ int bnxt_qplib_enable_rcfw_channel(struct pci_dev *pdev,
 	/* General */
 	rcfw->seq_num = 0;
 	set_bit(FIRMWARE_FIRST_FLAG, &rcfw->flags);
-	bmap_size = BITS_TO_LONGS(rcfw->cmdq_depth *
-				  sizeof(unsigned long));
+	bmap_size = BITS_TO_LONGS(rcfw->cmdq_depth) * sizeof(unsigned long);
 	rcfw->cmdq_bitmap = kzalloc(bmap_size, GFP_KERNEL);
 	if (!rcfw->cmdq_bitmap)
 		return -ENOMEM;
-- 
2.17.1

Re: [PATCH] RDMA/bnxt_re: fix a size calculation

[Devesh Sharma]

On Thu, Jan 10, 2019 at 6:30 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> This is from static analysis not from testing.  Depending on the value
> of rcfw->cmdq_depth, then this might not cause an issue at runtime.
>
> The BITS_TO_LONGS() macro tells us how many longs it take to hold a
> bitmap.  In other words, it divides by the number if bits per long and
> rounds up.  Then we want to take that number and multiple by
> sizeof(long) to get the number of bytes to allocate.
>
> The code here does the multiplication first so the rounding up is done
> in the wrong place.  So imagine we want to allocate 1 bit, then
> "(1 * 8) / 64 = 1" when we round up.  But it should be
> "(1 / 64) * 8 = 8".  In other words, because of the rounding difference
> we might allocate up to "sizeof(long) - 1" bytes fewer than intended.
>
> Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 326805461265..742346ea5b0d 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> @@ -684,8 +684,7 @@ int bnxt_qplib_enable_rcfw_channel(struct pci_dev *pdev,
>         /* General */
>         rcfw->seq_num = 0;
>         set_bit(FIRMWARE_FIRST_FLAG, &rcfw->flags);
> -       bmap_size = BITS_TO_LONGS(rcfw->cmdq_depth *
> -                                 sizeof(unsigned long));
> +       bmap_size = BITS_TO_LONGS(rcfw->cmdq_depth) * sizeof(unsigned long);
>         rcfw->cmdq_bitmap = kzalloc(bmap_size, GFP_KERNEL);
>         if (!rcfw->cmdq_bitmap)
>                 return -ENOMEM;
> --
> 2.17.1

Thanks for supplying this fix, it was on my list for my upcoming cleanup series.

Acked-By: Devesh Sharma <devesh.sharma@broadcom.com>

Re: [PATCH] RDMA/bnxt_re: fix a size calculation

[Jason Gunthorpe]

On Thu, Jan 10, 2019 at 04:00:19PM +0300, Dan Carpenter wrote:
> This is from static analysis not from testing.  Depending on the value
> of rcfw->cmdq_depth, then this might not cause an issue at runtime.
> 
> The BITS_TO_LONGS() macro tells us how many longs it take to hold a
> bitmap.  In other words, it divides by the number if bits per long and
> rounds up.  Then we want to take that number and multiple by
> sizeof(long) to get the number of bytes to allocate.
> 
> The code here does the multiplication first so the rounding up is done
> in the wrong place.  So imagine we want to allocate 1 bit, then
> "(1 * 8) / 64 = 1" when we round up.  But it should be
> "(1 / 64) * 8 = 8".  In other words, because of the rounding difference
> we might allocate up to "sizeof(long) - 1" bytes fewer than intended.
> 
> Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Acked-By: Devesh Sharma <devesh.sharma@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied to for-next

Thanks,
Jason