[v2] bnxt: avoid overflow in bnxt_get_nvram_directory()

  The value of an arithmetic expression is subject
of possible overflow due to a failure to cast operands to a larger data
type before performing arithmetic. Used macro for multiplication instead
operator for avoiding overflow.

Found by Security Code and Linux Verification
Center (linuxtesting.org) with SVACE.

Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
changelog:
- added "fixes" tag.
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Sun, Feb 19, 2023 at 11:46:56AM +0300, Maxim Korotkov wrote:
> The value of an arithmetic expression is subject
> of possible overflow due to a failure to cast operands to a larger data
> type before performing arithmetic. Used macro for multiplication instead
> operator for avoiding overflow.
> 
> Found by Security Code and Linux Verification
> Center (linuxtesting.org) with SVACE.
> 
> Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

I agree that it is correct to use mul_u32_u32() for multiplication
of two u32 entities where the result is 64bit, avoiding overflow.

And I agree that the fixes tag indicates the commit where the code
in question was introduced.

However, it is not clear to me if this is a theoretical bug
or one that can manifest in practice - I think it implies that
buflen really can be > 4Gbytes.

And thus it is not clear to me if this patch should be for 'net' or
'net-next'.

> ---
> changelog:
> - added "fixes" tag.
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index ec573127b707..696f32dfe41f 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -2862,7 +2862,7 @@ static int bnxt_get_nvram_directory(struct net_device *dev, u32 len, u8 *data)
>  	if (rc)
>  		return rc;
>  
> -	buflen = dir_entries * entry_length;
> +	buflen = mul_u32_u32(dir_entries, entry_length);
>  	buf = hwrm_req_dma_slice(bp, req, buflen, &dma_handle);
>  	if (!buf) {
>  		hwrm_req_drop(bp, req);
> -- 
> 2.37.2
>
  

On Sun, 2023-02-19 at 15:14 +0100, Simon Horman wrote:
> On Sun, Feb 19, 2023 at 11:46:56AM +0300, Maxim Korotkov wrote:
> > The value of an arithmetic expression is subject
> > of possible overflow due to a failure to cast operands to a larger data
> > type before performing arithmetic. Used macro for multiplication instead
> > operator for avoiding overflow.
> > 
> > Found by Security Code and Linux Verification
> > Center (linuxtesting.org) with SVACE.
> > 
> > Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> > Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
> > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> I agree that it is correct to use mul_u32_u32() for multiplication
> of two u32 entities where the result is 64bit, avoiding overflow.
> 
> And I agree that the fixes tag indicates the commit where the code
> in question was introduced.
> 
> However, it is not clear to me if this is a theoretical bug
> or one that can manifest in practice - I think it implies that
> buflen really can be > 4Gbytes.
> 
> And thus it is not clear to me if this patch should be for 'net' or
> 'net-next'.

... especially considered that both 'dir_entries' and 'entry_length'
are copied back to the user-space using a single byte each.

Cheers,

Paolo
  

On Tue, 2023-02-21 at 10:34 +0100, Paolo Abeni wrote:
> On Sun, 2023-02-19 at 15:14 +0100, Simon Horman wrote:
> > On Sun, Feb 19, 2023 at 11:46:56AM +0300, Maxim Korotkov wrote:
> > > The value of an arithmetic expression is subject
> > > of possible overflow due to a failure to cast operands to a larger data
> > > type before performing arithmetic. Used macro for multiplication instead
> > > operator for avoiding overflow.
> > > 
> > > Found by Security Code and Linux Verification
> > > Center (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
> > > Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
> > > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > 
> > I agree that it is correct to use mul_u32_u32() for multiplication
> > of two u32 entities where the result is 64bit, avoiding overflow.
> > 
> > And I agree that the fixes tag indicates the commit where the code
> > in question was introduced.
> > 
> > However, it is not clear to me if this is a theoretical bug
> > or one that can manifest in practice - I think it implies that
> > buflen really can be > 4Gbytes.
> > 
> > And thus it is not clear to me if this patch should be for 'net' or
> > 'net-next'.
> 
> ... especially considered that both 'dir_entries' and 'entry_length'
> are copied back to the user-space using a single byte each.

To be clear: if this is really a bug you should update the commit
message stating how the bug could happen. Otherwise you could repost
for net-next stripping the fixes tag.

Thanks,

Paolo
  

23.02.2023 11:01, Paolo Abeni wrote:
> On Tue, 2023-02-21 at 10:34 +0100, Paolo Abeni wrote:
>> On Sun, 2023-02-19 at 15:14 +0100, Simon Horman wrote:
>>> On Sun, Feb 19, 2023 at 11:46:56AM +0300, Maxim Korotkov wrote:
>>>> The value of an arithmetic expression is subject
>>>> of possible overflow due to a failure to cast operands to a larger data
>>>> type before performing arithmetic. Used macro for multiplication instead
>>>> operator for avoiding overflow.
>>>>
>>>> Found by Security Code and Linux Verification
>>>> Center (linuxtesting.org) with SVACE.
>>>>
>>>> Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
>>>> Signed-off-by: Maxim Korotkov <korotkov.maxim.s@gmail.com>
>>>> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>>>
>>> I agree that it is correct to use mul_u32_u32() for multiplication
>>> of two u32 entities where the result is 64bit, avoiding overflow.
>>>
>>> And I agree that the fixes tag indicates the commit where the code
>>> in question was introduced.
>>>
>>> However, it is not clear to me if this is a theoretical bug
>>> or one that can manifest in practice - I think it implies that
>>> buflen really can be > 4Gbytes.
>>>
>>> And thus it is not clear to me if this patch should be for 'net' or
>>> 'net-next'.
>>
>> ... especially considered that both 'dir_entries' and 'entry_length'
>> are copied back to the user-space using a single byte each.
> 
> To be clear: if this is really a bug you should update the commit
> message stating how the bug could happen. Otherwise you could repost
> for net-next stripping the fixes tag.
> 
> Thanks,
> 
> Paolo
> 
This is more of a hypothetical issue in my opinion. At least I don't 
have proof of concept. I'll resend this patch when net-next tree be open.
Best regards, Max