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 <[email protected]>
Reviewed-by: Pavan Chebbi <[email protected]>
---
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, 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 <[email protected]>
> Reviewed-by: Pavan Chebbi <[email protected]>
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 <[email protected]>
> > Reviewed-by: Pavan Chebbi <[email protected]>
>
> 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 <[email protected]>
> > > Reviewed-by: Pavan Chebbi <[email protected]>
> >
> > 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 <[email protected]>
>>>> Reviewed-by: Pavan Chebbi <[email protected]>
>>>
>>> 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