[PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Ivan Vecera]

Currently when PF administratively sets VF's MAC address and the VF
is put down (VF tries to delete all MACs) then the MAC is removed
from MAC filters and primary VF MAC is zeroed.

Do not allow untrusted VF to remove primary MAC when it was set
administratively by PF.

Reproducer:
1) Create VF
2) Set VF interface up
3) Administratively set the VF's MAC
4) Put VF interface down

[root@host ~]# echo 1 > /sys/class/net/enp2s0f0/device/sriov_numvfs
[root@host ~]# ip link set enp2s0f0v0 up
[root@host ~]# ip link set enp2s0f0 vf 0 mac fe:6c:b5:da:c7:7d
[root@host ~]# ip link show enp2s0f0
23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
    vf 0     link/ether fe:6c:b5:da:c7:7d brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
[root@host ~]# ip link set enp2s0f0v0 down
[root@host ~]# ip link show enp2s0f0
23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
    vf 0     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off

Fixes: 700bbf6c1f9e ("i40e: allow VF to remove any MAC filter")
Fixes: ceb29474bbbc ("i40e: Add support for VF to specify its primary MAC address")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 38 ++++++++++++++++---
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 908cdbd3ec5d..b34c71770887 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2848,6 +2848,24 @@ static int i40e_vc_get_stats_msg(struct i40e_vf *vf, u8 *msg)
 				      (u8 *)&stats, sizeof(stats));
 }
 
+/**
+ * i40e_can_vf_change_mac
+ * @vf: pointer to the VF info
+ *
+ * Return true if the VF is allowed to change its MAC filters, false otherwise
+ */
+static bool i40e_can_vf_change_mac(struct i40e_vf *vf)
+{
+	/* If the VF MAC address has been set administratively (via the
+	 * ndo_set_vf_mac command), then deny permission to the VF to
+	 * add/delete unicast MAC addresses, unless the VF is trusted
+	 */
+	if (vf->pf_set_mac && !vf->trusted)
+		return false;
+
+	return true;
+}
+
 #define I40E_MAX_MACVLAN_PER_HW 3072
 #define I40E_MAX_MACVLAN_PER_PF(num_ports) (I40E_MAX_MACVLAN_PER_HW /	\
 	(num_ports))
@@ -2907,8 +2925,8 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 		 * The VF may request to set the MAC address filter already
 		 * assigned to it so do not return an error in that case.
 		 */
-		if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
-		    !is_multicast_ether_addr(addr) && vf->pf_set_mac &&
+		if (!i40e_can_vf_change_mac(vf) &&
+		    !is_multicast_ether_addr(addr) &&
 		    !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
 			dev_err(&pf->pdev->dev,
 				"VF attempting to override administratively set MAC address, bring down and up the VF interface to resume normal operation\n");
@@ -3114,19 +3132,29 @@ static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 			ret = -EINVAL;
 			goto error_param;
 		}
-		if (ether_addr_equal(al->list[i].addr, vf->default_lan_addr.addr))
-			was_unimac_deleted = true;
 	}
 	vsi = pf->vsi[vf->lan_vsi_idx];
 
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 	/* delete addresses from the list */
-	for (i = 0; i < al->num_elements; i++)
+	for (i = 0; i < al->num_elements; i++) {
+		const u8 *addr = al->list[i].addr;
+
+		/* Allow to delete VF primary MAC only if it was not set
+		 * administratively by PF or if VF is trusted.
+		 */
+		if (ether_addr_equal(addr, vf->default_lan_addr.addr) &&
+		    i40e_can_vf_change_mac(vf))
+			was_unimac_deleted = true;
+		else
+			continue;
+
 		if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
 			ret = -EINVAL;
 			spin_unlock_bh(&vsi->mac_filter_hash_lock);
 			goto error_param;
 		}
+	}
 
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
 
-- 
2.39.3

Re: [PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Simon Horman]

On Wed, Jan 31, 2024 at 02:17:14PM +0100, Ivan Vecera wrote:
> Currently when PF administratively sets VF's MAC address and the VF
> is put down (VF tries to delete all MACs) then the MAC is removed
> from MAC filters and primary VF MAC is zeroed.
> 
> Do not allow untrusted VF to remove primary MAC when it was set
> administratively by PF.
> 
> Reproducer:
> 1) Create VF
> 2) Set VF interface up
> 3) Administratively set the VF's MAC
> 4) Put VF interface down
> 
> [root@host ~]# echo 1 > /sys/class/net/enp2s0f0/device/sriov_numvfs
> [root@host ~]# ip link set enp2s0f0v0 up
> [root@host ~]# ip link set enp2s0f0 vf 0 mac fe:6c:b5:da:c7:7d
> [root@host ~]# ip link show enp2s0f0
> 23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>     link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
>     vf 0     link/ether fe:6c:b5:da:c7:7d brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
> [root@host ~]# ip link set enp2s0f0v0 down
> [root@host ~]# ip link show enp2s0f0
> 23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>     link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
>     vf 0     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
> 
> Fixes: 700bbf6c1f9e ("i40e: allow VF to remove any MAC filter")
> Fixes: ceb29474bbbc ("i40e: Add support for VF to specify its primary MAC address")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>

Thanks Ivan,

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Tony Nguyen]

From: Ivan Vecera <ivecera@redhat.com>

Currently when PF administratively sets VF's MAC address and the VF
is put down (VF tries to delete all MACs) then the MAC is removed
from MAC filters and primary VF MAC is zeroed.

Do not allow untrusted VF to remove primary MAC when it was set
administratively by PF.

Reproducer:
1) Create VF
2) Set VF interface up
3) Administratively set the VF's MAC
4) Put VF interface down

[root@host ~]# echo 1 > /sys/class/net/enp2s0f0/device/sriov_numvfs
[root@host ~]# ip link set enp2s0f0v0 up
[root@host ~]# ip link set enp2s0f0 vf 0 mac fe:6c:b5:da:c7:7d
[root@host ~]# ip link show enp2s0f0
23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
    vf 0     link/ether fe:6c:b5:da:c7:7d brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
[root@host ~]# ip link set enp2s0f0v0 down
[root@host ~]# ip link show enp2s0f0
23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
    vf 0     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off

Fixes: 700bbf6c1f9e ("i40e: allow VF to remove any MAC filter")
Fixes: ceb29474bbbc ("i40e: Add support for VF to specify its primary MAC address")
Signed-off-by: Ivan Vecera <ivecera@redhat.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 38 ++++++++++++++++---
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 908cdbd3ec5d..b34c71770887 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2848,6 +2848,24 @@ static int i40e_vc_get_stats_msg(struct i40e_vf *vf, u8 *msg)
 				      (u8 *)&stats, sizeof(stats));
 }
 
+/**
+ * i40e_can_vf_change_mac
+ * @vf: pointer to the VF info
+ *
+ * Return true if the VF is allowed to change its MAC filters, false otherwise
+ */
+static bool i40e_can_vf_change_mac(struct i40e_vf *vf)
+{
+	/* If the VF MAC address has been set administratively (via the
+	 * ndo_set_vf_mac command), then deny permission to the VF to
+	 * add/delete unicast MAC addresses, unless the VF is trusted
+	 */
+	if (vf->pf_set_mac && !vf->trusted)
+		return false;
+
+	return true;
+}
+
 #define I40E_MAX_MACVLAN_PER_HW 3072
 #define I40E_MAX_MACVLAN_PER_PF(num_ports) (I40E_MAX_MACVLAN_PER_HW /	\
 	(num_ports))
@@ -2907,8 +2925,8 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 		 * The VF may request to set the MAC address filter already
 		 * assigned to it so do not return an error in that case.
 		 */
-		if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
-		    !is_multicast_ether_addr(addr) && vf->pf_set_mac &&
+		if (!i40e_can_vf_change_mac(vf) &&
+		    !is_multicast_ether_addr(addr) &&
 		    !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
 			dev_err(&pf->pdev->dev,
 				"VF attempting to override administratively set MAC address, bring down and up the VF interface to resume normal operation\n");
@@ -3114,19 +3132,29 @@ static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 			ret = -EINVAL;
 			goto error_param;
 		}
-		if (ether_addr_equal(al->list[i].addr, vf->default_lan_addr.addr))
-			was_unimac_deleted = true;
 	}
 	vsi = pf->vsi[vf->lan_vsi_idx];
 
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 	/* delete addresses from the list */
-	for (i = 0; i < al->num_elements; i++)
+	for (i = 0; i < al->num_elements; i++) {
+		const u8 *addr = al->list[i].addr;
+
+		/* Allow to delete VF primary MAC only if it was not set
+		 * administratively by PF or if VF is trusted.
+		 */
+		if (ether_addr_equal(addr, vf->default_lan_addr.addr) &&
+		    i40e_can_vf_change_mac(vf))
+			was_unimac_deleted = true;
+		else
+			continue;
+
 		if (i40e_del_mac_filter(vsi, al->list[i].addr)) {
 			ret = -EINVAL;
 			spin_unlock_bh(&vsi->mac_filter_hash_lock);
 			goto error_param;
 		}
+	}
 
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
 
-- 
2.41.0

Re: [PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Tony Nguyen]

On 2/8/2024 10:03 AM, Tony Nguyen wrote:
> From: Ivan Vecera <ivecera@redhat.com>
> 
> Currently when PF administratively sets VF's MAC address and the VF
> is put down (VF tries to delete all MACs) then the MAC is removed
> from MAC filters and primary VF MAC is zeroed.
> 
> Do not allow untrusted VF to remove primary MAC when it was set
> administratively by PF.

This is currently marked as "Not Applicable" [1]. Are there changes to 
be done or, perhaps, it got mismarked? If the latter, I do have an i40e 
pull request to send so I could also bundle this with that if it's more 
convenient.

Thanks,
Tony

> Reproducer:
> 1) Create VF
> 2) Set VF interface up
> 3) Administratively set the VF's MAC
> 4) Put VF interface down
> 
> [root@host ~]# echo 1 > /sys/class/net/enp2s0f0/device/sriov_numvfs
> [root@host ~]# ip link set enp2s0f0v0 up
> [root@host ~]# ip link set enp2s0f0 vf 0 mac fe:6c:b5:da:c7:7d
> [root@host ~]# ip link show enp2s0f0
> 23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>      link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
>      vf 0     link/ether fe:6c:b5:da:c7:7d brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
> [root@host ~]# ip link set enp2s0f0v0 down
> [root@host ~]# ip link show enp2s0f0
> 23: enp2s0f0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
>      link/ether 3c:ec:ef:b7:dd:04 brd ff:ff:ff:ff:ff:ff
>      vf 0     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff, spoof checking on, link-state auto, trust off
> 
> Fixes: 700bbf6c1f9e ("i40e: allow VF to remove any MAC filter")
> Fixes: ceb29474bbbc ("i40e: Add support for VF to specify its primary MAC address")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> Reviewed-by: Simon Horman <horms@kernel.org>
> Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

[1] 
https://patchwork.kernel.org/project/netdevbpf/patch/20240208180335.1844996-1-anthony.l.nguyen@intel.com/

Re: [PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Jakub Kicinski]

On Mon, 12 Feb 2024 10:11:44 -0800 Tony Nguyen wrote:
> > Currently when PF administratively sets VF's MAC address and the VF
> > is put down (VF tries to delete all MACs) then the MAC is removed
> > from MAC filters and primary VF MAC is zeroed.
> > 
> > Do not allow untrusted VF to remove primary MAC when it was set
> > administratively by PF.  
> 
> This is currently marked as "Not Applicable" [1]. Are there changes to 
> be done or, perhaps, it got mismarked? If the latter, I do have an i40e 
> pull request to send so I could also bundle this with that if it's more 
> convenient.

I'm guessing mismarked.

Re: [PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[Tony Nguyen]

On 2/12/2024 4:37 PM, Jakub Kicinski wrote:
> On Mon, 12 Feb 2024 10:11:44 -0800 Tony Nguyen wrote:
>>> Currently when PF administratively sets VF's MAC address and the VF
>>> is put down (VF tries to delete all MACs) then the MAC is removed
>>> from MAC filters and primary VF MAC is zeroed.
>>>
>>> Do not allow untrusted VF to remove primary MAC when it was set
>>> administratively by PF.
>>
>> This is currently marked as "Not Applicable" [1]. Are there changes to
>> be done or, perhaps, it got mismarked? If the latter, I do have an i40e
>> pull request to send so I could also bundle this with that if it's more
>> convenient.
> 
> I'm guessing mismarked.

I see it as Under Review now. I'll go ahead and do the other patches 
without this one included then.

Thanks,
Tony

Re: [PATCH net] i40e: Do not allow untrusted VF to remove administratively set MAC

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  8 Feb 2024 10:03:33 -0800 you wrote:
> From: Ivan Vecera <ivecera@redhat.com>
> 
> Currently when PF administratively sets VF's MAC address and the VF
> is put down (VF tries to delete all MACs) then the MAC is removed
> from MAC filters and primary VF MAC is zeroed.
> 
> Do not allow untrusted VF to remove primary MAC when it was set
> administratively by PF.
> 
> [...]

Here is the summary with links:
  - [net] i40e: Do not allow untrusted VF to remove administratively set MAC
    https://git.kernel.org/netdev/net/c/73d9629e1c8c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html