be2net: Remove potential access to the zero address

[PATCH] be2net: Remove potential access to the zero address

Posted by Ваторопин Андрей 8 months, 1 week ago

From: Andrey Vatoropin <a.vatoropin@crpt.ru>

At the moment of calling the function be_cmd_get_mac_from_list() with the
following parameters:
be_cmd_get_mac_from_list(adapter, mac, &pmac_valid, NULL, 
					adapter->if_handle, 0);

The parameter "pmac_id" equals NULL.

Then, if "mac_addr_size" equals four bytes, there is a possibility of
accessing the zero address via the pointer "pmac_id".

Add an extra check for the pointer "pmac_id" to avoid accessing the zero
address.

Found by Linux Verification Center (linuxtesting.org) with SVACE.
       
Fixes: e5e1ee894615 ("be2net: Use new implementation of get mac list command")
Signed-off-by: Andrey Vatoropin <a.vatoropin@crpt.ru>
---
 drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
index 51b8377edd1d..be5bbf6881b8 100644
--- a/drivers/net/ethernet/emulex/benet/be_cmds.c
+++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
@@ -3757,7 +3757,7 @@ int be_cmd_get_mac_from_list(struct be_adapter *adapter, u8 *mac,
 			/* mac_id is a 32 bit value and mac_addr size
 			 * is 6 bytes
 			 */
-			if (mac_addr_size == sizeof(u32)) {
+			if (pmac_id && mac_addr_size == sizeof(u32)) {
 				*pmac_id_valid = true;
 				mac_id = mac_entry->mac_addr_id.s_mac_id.mac_id;
 				*pmac_id = le32_to_cpu(mac_id);
-- 
2.43.0

Re: [PATCH] be2net: Remove potential access to the zero address

Posted by Michal Swiatkowski 8 months, 1 week ago

On Wed, Apr 16, 2025 at 10:55:47AM +0000, Ваторопин Андрей wrote:
> From: Andrey Vatoropin <a.vatoropin@crpt.ru>
> 
> At the moment of calling the function be_cmd_get_mac_from_list() with the
> following parameters:
> be_cmd_get_mac_from_list(adapter, mac, &pmac_valid, NULL, 
> 					adapter->if_handle, 0);

Looks like pmac_valid needs to be false to reach *pmac_id assign.

> 
> The parameter "pmac_id" equals NULL.
> 
> Then, if "mac_addr_size" equals four bytes, there is a possibility of
> accessing the zero address via the pointer "pmac_id".
> 
> Add an extra check for the pointer "pmac_id" to avoid accessing the zero
> address.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>        
> Fixes: e5e1ee894615 ("be2net: Use new implementation of get mac list command")
> Signed-off-by: Andrey Vatoropin <a.vatoropin@crpt.ru>
> ---
>  drivers/net/ethernet/emulex/benet/be_cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c
> index 51b8377edd1d..be5bbf6881b8 100644
> --- a/drivers/net/ethernet/emulex/benet/be_cmds.c
> +++ b/drivers/net/ethernet/emulex/benet/be_cmds.c
> @@ -3757,7 +3757,7 @@ int be_cmd_get_mac_from_list(struct be_adapter *adapter, u8 *mac,
>  			/* mac_id is a 32 bit value and mac_addr size
>  			 * is 6 bytes
>  			 */
> -			if (mac_addr_size == sizeof(u32)) {
> +			if (pmac_id && mac_addr_size == sizeof(u32)) {
>  				*pmac_id_valid = true;
>  				mac_id = mac_entry->mac_addr_id.s_mac_id.mac_id;
>  				*pmac_id = le32_to_cpu(mac_id);

Thanks for fixing.
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.43.0

Re: [PATCH] be2net: Remove potential access to the zero address

Posted by Jakub Kicinski 8 months ago

On Wed, 16 Apr 2025 13:32:29 +0200 Michal Swiatkowski wrote:
> > At the moment of calling the function be_cmd_get_mac_from_list() with the
> > following parameters:
> > be_cmd_get_mac_from_list(adapter, mac, &pmac_valid, NULL, 
> > 					adapter->if_handle, 0);  
> 
> Looks like pmac_valid needs to be false to reach *pmac_id assign.

Right, it is for this caller and there is a check which skip this logic
if pmac_id_valid is false, line 3738.

Re: [PATCH] be2net: Remove potential access to the zero address

Posted by Fedor Pchelkin 8 months ago

On Thu, 17. Apr 19:54, Jakub Kicinski wrote:
> On Wed, 16 Apr 2025 13:32:29 +0200 Michal Swiatkowski wrote:
> > > At the moment of calling the function be_cmd_get_mac_from_list() with the
> > > following parameters:
> > > be_cmd_get_mac_from_list(adapter, mac, &pmac_valid, NULL, 
> > > 					adapter->if_handle, 0);  
> > 
> > Looks like pmac_valid needs to be false to reach *pmac_id assign.
> 
> Right, it is for this caller and there is a check which skip this logic
> if pmac_id_valid is false, line 3738.

Wait, the check you are referring to is

	if (*pmac_id_valid) {
		memcpy(mac, resp->macid_macaddr.mac_addr_id.macaddr,
		       ETH_ALEN);
		goto out;
	}

which will skip that part only if pmac_id_valid is *true*.

Re: [PATCH] be2net: Remove potential access to the zero address

Posted by Jakub Kicinski 8 months ago

On Fri, 18 Apr 2025 10:50:43 +0300 Fedor Pchelkin wrote:
> On Thu, 17. Apr 19:54, Jakub Kicinski wrote:
> > On Wed, 16 Apr 2025 13:32:29 +0200 Michal Swiatkowski wrote:  
> > > > At the moment of calling the function be_cmd_get_mac_from_list() with the
> > > > following parameters:
> > > > be_cmd_get_mac_from_list(adapter, mac, &pmac_valid, NULL, 
> > > > 					adapter->if_handle, 0);    
> > > 
> > > Looks like pmac_valid needs to be false to reach *pmac_id assign.  
> > 
> > Right, it is for this caller and there is a check which skip this logic
> > if pmac_id_valid is false, line 3738.  
> 
> Wait, the check you are referring to is

Ugh, I'm blind. The fix is too.. poor, tho.
Why are we in this loop at all if we masked out the only break
condition.