[Intel-wired-lan] [next PATCH S87 V2 3/7] i40e: Fix permission check for VF MAC filters

[Shannon Nelson <shannon.nelson@oracle.com>]

On 2/26/2018 12:33 AM, Alice Michael wrote:
> From: Filip Sadowski <filip.sadowski@intel.com>
> 
> When VF requests adding of MAC filters the checking is done against number
> of already present MAC filters not adding them at the same time. It makes
> it possible to add a bunch of filters at once possibly exceeding
> acceptable limit of I40E_VC_MAX_MAC_ADDR_PER_VF filters.
> 
> This happens because when checking vf->num_mac, we do not check how many
> filters are being requested at once. Modify the check function to ensure
> that it knows how many filters are being requested. This allows the
> check to ensure that the total number of filters in a single request
> does not cause us to go over the limit.
> 
> Additionally, move the check to within the lock to ensure that the
> vf->num_mac is checked while holding the lock to maintain consistency.
> We could have simply moved the call to i40e_vf_check_permission to
> within the loop, but this could cause a request to be non-atomic, and
> add some but not all the addresses, while reporting an error code. We
> want to avoid this behavior so that users are not confused about which
> filters have or have not been added.
> 
> Signed-off-by: Filip Sadowski <filip.sadowski@intel.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 79 ++++++++++++++--------
>   1 file changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 2de7a8c..c7c6c4b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -2369,25 +2369,47 @@ static int i40e_vc_get_stats_msg(struct i40e_vf *vf, u8 *msg, u16 msglen)
>   /**
>    * i40e_check_vf_permission
>    * @vf: pointer to the VF info
> - * @macaddr: pointer to the MAC Address being checked
> + * @al: MAC address list from virtchnl
>    *
> - * Check if the VF has permission to add or delete unicast MAC address
> - * filters and return error code -EPERM if not.  Then check if the
> - * address filter requested is broadcast or zero and if so return
> - * an invalid MAC address error code.
> + * Check that the given list of MAC addresses is allowed. Will return -EPERM
> + * if any address in the list is not valid. Checks the following conditions:
> + *
> + * 1) broadcast and zero addresses are never valid
> + * 2) unicast addresses are not allowed if the VMM has administratively set
> + *    the VF MAC address, unless the VF is marked as privileged.
> + * 3) There is enough space to add all the addresses.
> + *
> + * Note that to guarantee consistency, it is expected this function be called
> + * while holding the mac_filter_hash_lock, as otherwise the current number of
> + * addresses might not be accurate.
>    **/
> -static inline int i40e_check_vf_permission(struct i40e_vf *vf, u8 *macaddr)
> +static inline int i40e_check_vf_permission(struct i40e_vf *vf,
> +					   struct virtchnl_ether_addr_list *al)
>   {
>   	struct i40e_pf *pf = vf->pf;
> -	int ret = 0;
> +	int i;
> +
> +	/* If this VF is not privileged, then we can't add more than a limited
> +	 * number of addresses. Check to make sure that the additions do not
> +	 * push us over the limit.
> +	 */
> +	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
> +	    (vf->num_mac + al->num_elements) > I40E_VC_MAX_MAC_ADDR_PER_VF) {
> +		dev_err(&pf->pdev->dev,
> +			"VF is not trusted, switch the VF to trusted to add more functionality\n");

This error message should have wording that makes it clear this came 
from a request to add more MAC addresses, similar to the error message 
below.

sln

> +		return -EPERM;
> +	}
> +
> +	for (i = 0; i < al->num_elements; i++) {
> +		u8 *addr = al->list[i].addr;
> +
> +		if (is_broadcast_ether_addr(addr) ||
> +		    is_zero_ether_addr(addr)) {
> +			dev_err(&pf->pdev->dev, "invalid VF MAC addr %pM\n",
> +				addr);
> +			return I40E_ERR_INVALID_MAC_ADDR;
> +		}
>   
> -	if (is_broadcast_ether_addr(macaddr) ||
> -		   is_zero_ether_addr(macaddr)) {
> -		dev_err(&pf->pdev->dev, "invalid VF MAC addr %pM\n", macaddr);
> -		ret = I40E_ERR_INVALID_MAC_ADDR;
> -	} else if (vf->pf_set_mac && !is_multicast_ether_addr(macaddr) &&
> -		   !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
> -		   !ether_addr_equal(macaddr, vf->default_lan_addr.addr)) {
>   		/* If the host VMM administrator has set the VF MAC address
>   		 * administratively via the ndo_set_vf_mac command then deny
>   		 * permission to the VF to add or delete unicast MAC addresses.
> @@ -2395,16 +2417,16 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf, u8 *macaddr)
>   		 * The VF may request to set the MAC address filter already
>   		 * assigned to it so do not return an error in that case.
>   		 */
> -		dev_err(&pf->pdev->dev,
> -			"VF attempting to override administratively set MAC address, reload the VF driver to resume normal operation\n");
> -		ret = -EPERM;
> -	} else if ((vf->num_mac >= I40E_VC_MAX_MAC_ADDR_PER_VF) &&
> -		   !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
> -		dev_err(&pf->pdev->dev,
> -			"VF is not trusted, switch the VF to trusted to add more functionality\n");
> -		ret = -EPERM;
> +		if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
> +		    !is_multicast_ether_addr(addr) && vf->pf_set_mac &&
> +		    !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
> +			dev_err(&pf->pdev->dev,
> +				"VF attempting to override administratively set MAC address, reload the VF driver to resume normal operation\n");
> +			return -EPERM;
> +		}
>   	}
> -	return ret;
> +
> +	return 0;
>   }
>   
>   /**
> @@ -2431,11 +2453,6 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg, u16 msglen)
>   		goto error_param;
>   	}
>   
> -	for (i = 0; i < al->num_elements; i++) {
> -		ret = i40e_check_vf_permission(vf, al->list[i].addr);
> -		if (ret)
> -			goto error_param;
> -	}
>   	vsi = pf->vsi[vf->lan_vsi_idx];
>   
>   	/* Lock once, because all function inside for loop accesses VSI's
> @@ -2443,6 +2460,12 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg, u16 msglen)
>   	 */
>   	spin_lock_bh(&vsi->mac_filter_hash_lock);
>   
> +	ret = i40e_check_vf_permission(vf, al);
> +	if (ret) {
> +		spin_unlock_bh(&vsi->mac_filter_hash_lock);
> +		goto error_param;
> +	}
> +
>   	/* add new addresses to the list */
>   	for (i = 0; i < al->num_elements; i++) {
>   		struct i40e_mac_filter *f;
>