[PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Yuran Pereira]

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 <yuran.pereira@hotmail.com>
---
 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

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Greg KH]

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 <yuran.pereira@hotmail.com>
> ---
>  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

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Yuran Pereira]

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

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Yuran Pereira]

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);

```

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Yuran Pereira]

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);

```

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2055 bytes --]



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 <yuran.pereira@hotmail.com>
>> ---
>>   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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH] Prevent out-of-bounds read/write in bcmasp_netfilt_rd and bcmasp_netfilt_wr

[Greg KH]

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 <yuran.pereira@hotmail.com>
> > > ---
> > >   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