The functions `bcmasp_netfilt_rd` and `bcmasp_netfilt_wr` both call
`bcmasp_netfilt_get_reg_offset` which, when it fails, returns `-EINVAL`.
This could lead to an out-of-bounds read or write when `rx_filter_core_rl`
or `rx_filter_core_wl` is called.
This patch adds a check in both functions to return immediately if
`bcmasp_netfilt_get_reg_offset` fails. This prevents potential out-of-bounds read
or writes, and ensures that no undefined or buggy behavior would originate from
the failure of `bcmasp_netfilt_get_reg_offset`.
Addresses-Coverity-IDs: 1544536 ("Out-of-bounds access")
Signed-off-by: Yuran Pereira <[email protected]>
---
drivers/net/ethernet/broadcom/asp2/bcmasp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
index 29b04a274d07..8b90b761bdec 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
@@ -227,6 +227,8 @@ static void bcmasp_netfilt_wr(struct bcmasp_priv *priv,
reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
offset);
+ if (reg_offset < 0)
+ return;
rx_filter_core_wl(priv, val, reg_offset);
}
@@ -244,6 +246,8 @@ static u32 bcmasp_netfilt_rd(struct bcmasp_priv *priv,
reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
offset);
+ if (reg_offset < 0)
+ return 0;
return rx_filter_core_rl(priv, reg_offset);
}
--
2.25.1
On Fri, Nov 03, 2023 at 05:57:48PM +0530, Yuran Pereira wrote:
> The functions `bcmasp_netfilt_rd` and `bcmasp_netfilt_wr` both call
> `bcmasp_netfilt_get_reg_offset` which, when it fails, returns `-EINVAL`.
> This could lead to an out-of-bounds read or write when `rx_filter_core_rl`
> or `rx_filter_core_wl` is called.
>
> This patch adds a check in both functions to return immediately if
> `bcmasp_netfilt_get_reg_offset` fails. This prevents potential out-of-bounds read
> or writes, and ensures that no undefined or buggy behavior would originate from
> the failure of `bcmasp_netfilt_get_reg_offset`.
>
> Addresses-Coverity-IDs: 1544536 ("Out-of-bounds access")
> Signed-off-by: Yuran Pereira <[email protected]>
> ---
> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 29b04a274d07..8b90b761bdec 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> @@ -227,6 +227,8 @@ static void bcmasp_netfilt_wr(struct bcmasp_priv *priv,
>
> reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
> offset);
> + if (reg_offset < 0)
> + return;
>
> rx_filter_core_wl(priv, val, reg_offset);
> }
> @@ -244,6 +246,8 @@ static u32 bcmasp_netfilt_rd(struct bcmasp_priv *priv,
>
> reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
> offset);
> + if (reg_offset < 0)
> + return 0;
Shouldn't you return an error here?
thanks
greg k-h
Hello Greg,
On Fri, Nov 03, 2023 at 01:57:13PM +0100, Greg KH wrote:
> > reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
> > offset);
> > + if (reg_offset < 0)
> > + return 0;
>
> Shouldn't you return an error here?
Yes, I think that makes sense. I might just return `reg_offset`
since it is bound to be -EINVAL when bcmasp_netfilt_get_reg_offset
fails.
But that now makes me wonder whether the previous check in that
function which currently returns 0, shouldn't be returning `-EINVAL`
instead.
```
static u32 bcmasp_netfilt_rd(struct bcmasp_priv *priv,
...
{
if (!IS_ALIGNED(offset, 4) || offset > MAX_WAKE_FILTER_SIZE)
return 0; <----- Should this one be -EINVAL?
}
```
Thank you for the feedback.
Yuran
I guess that explains why the first check returns 0.
```
static int bcmasp_netfilt_wr_m_wake(struct bcmasp_priv *priv,
...
{
...
if (first_byte && (!IS_ALIGNED(offset, 4) || size < 3)) {
match_val = bcmasp_netfilt_rd(priv, nfilt,
ASP_NETFILT_MATCH,
ALIGN_DOWN(offset, 4));
mask_val = bcmasp_netfilt_rd(priv, nfilt,
ASP_NETFILT_MASK,
ALIGN_DOWN(offset, 4));
}
shift = (3 - (offset % 4)) * 8;
match_val &= ~GENMASK(shift + 7, shift);
mask_val &= ~GENMASK(shift + 7, shift);
match_val |= (u32)(*((u8 *)match) << shift);
mask_val |= (u32)(*((u8 *)mask) << shift);
```
On a second thought, it might not be a good idea to return
an error without modifying the caller, since the caller of
this function currently uses this return value without checking
if it's an error.
I guess that explains why the first check returns 0.
```
static int bcmasp_netfilt_wr_m_wake(struct bcmasp_priv *priv,
...
{
...
if (first_byte && (!IS_ALIGNED(offset, 4) || size < 3)) {
match_val = bcmasp_netfilt_rd(priv, nfilt,
ASP_NETFILT_MATCH,
ALIGN_DOWN(offset, 4));
mask_val = bcmasp_netfilt_rd(priv, nfilt,
ASP_NETFILT_MASK,
ALIGN_DOWN(offset, 4));
}
shift = (3 - (offset % 4)) * 8;
match_val &= ~GENMASK(shift + 7, shift);
mask_val &= ~GENMASK(shift + 7, shift);
match_val |= (u32)(*((u8 *)match) << shift);
mask_val |= (u32)(*((u8 *)mask) << shift);
```
On 11/3/23 5:57 AM, Greg KH wrote:
> On Fri, Nov 03, 2023 at 05:57:48PM +0530, Yuran Pereira wrote:
>> The functions `bcmasp_netfilt_rd` and `bcmasp_netfilt_wr` both call
>> `bcmasp_netfilt_get_reg_offset` which, when it fails, returns `-EINVAL`.
>> This could lead to an out-of-bounds read or write when `rx_filter_core_rl`
>> or `rx_filter_core_wl` is called.
>>
>> This patch adds a check in both functions to return immediately if
>> `bcmasp_netfilt_get_reg_offset` fails. This prevents potential out-of-bounds read
>> or writes, and ensures that no undefined or buggy behavior would originate from
>> the failure of `bcmasp_netfilt_get_reg_offset`.
>>
>> Addresses-Coverity-IDs: 1544536 ("Out-of-bounds access")
>> Signed-off-by: Yuran Pereira <[email protected]>
>> ---
>> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> index 29b04a274d07..8b90b761bdec 100644
>> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
>> @@ -227,6 +227,8 @@ static void bcmasp_netfilt_wr(struct bcmasp_priv *priv,
>>
>> reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
>> offset);
>> + if (reg_offset < 0)
>> + return;
>>
>> rx_filter_core_wl(priv, val, reg_offset);
>> }
>> @@ -244,6 +246,8 @@ static u32 bcmasp_netfilt_rd(struct bcmasp_priv *priv,
>>
>> reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
>> offset);
>> + if (reg_offset < 0)
>> + return 0;
>
> Shouldn't you return an error here?
>
> thanks
>
> greg k-h
As long as offset is less than MAX_WAKE_FILTER_SIZE we don't need to
worry about error checking. This is already checked before we call
netfilt_get_reg_offset() in both cases. Instead of returning -EINVAL in
neffilt_get_reg_offset() lets return 0. This will silence the coverity
check. In practice we will never hit this logic.
Thanks,
Justin
On Fri, Nov 03, 2023 at 11:23:16AM -0700, Justin Chen wrote:
>
>
> On 11/3/23 5:57 AM, Greg KH wrote:
> > On Fri, Nov 03, 2023 at 05:57:48PM +0530, Yuran Pereira wrote:
> > > The functions `bcmasp_netfilt_rd` and `bcmasp_netfilt_wr` both call
> > > `bcmasp_netfilt_get_reg_offset` which, when it fails, returns `-EINVAL`.
> > > This could lead to an out-of-bounds read or write when `rx_filter_core_rl`
> > > or `rx_filter_core_wl` is called.
> > >
> > > This patch adds a check in both functions to return immediately if
> > > `bcmasp_netfilt_get_reg_offset` fails. This prevents potential out-of-bounds read
> > > or writes, and ensures that no undefined or buggy behavior would originate from
> > > the failure of `bcmasp_netfilt_get_reg_offset`.
> > >
> > > Addresses-Coverity-IDs: 1544536 ("Out-of-bounds access")
> > > Signed-off-by: Yuran Pereira <[email protected]>
> > > ---
> > > drivers/net/ethernet/broadcom/asp2/bcmasp.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> > > index 29b04a274d07..8b90b761bdec 100644
> > > --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> > > +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> > > @@ -227,6 +227,8 @@ static void bcmasp_netfilt_wr(struct bcmasp_priv *priv,
> > > reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
> > > offset);
> > > + if (reg_offset < 0)
> > > + return;
> > > rx_filter_core_wl(priv, val, reg_offset);
> > > }
> > > @@ -244,6 +246,8 @@ static u32 bcmasp_netfilt_rd(struct bcmasp_priv *priv,
> > > reg_offset = bcmasp_netfilt_get_reg_offset(priv, nfilt, reg_type,
> > > offset);
> > > + if (reg_offset < 0)
> > > + return 0;
> >
> > Shouldn't you return an error here?
> >
> > thanks
> >
> > greg k-h
>
> As long as offset is less than MAX_WAKE_FILTER_SIZE we don't need to worry
> about error checking. This is already checked before we call
> netfilt_get_reg_offset() in both cases. Instead of returning -EINVAL in
> neffilt_get_reg_offset() lets return 0. This will silence the coverity
> check. In practice we will never hit this logic.
Then don't change it, coverity is incorrect here.
greg k-h