Re: [PATCH net v2 1/1] i40e: i40e_main: fix a missing check on list iterator

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 10 May 2022 13:48:46 -0700 Tony Nguyen wrote:
> From: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> 
> The bug is here:
> 	ret = i40e_add_macvlan_filter(hw, ch->seid, vdev->dev_addr, &aq_err);
> 
> The list iterator 'ch' will point to a bogus position containing
> HEAD if the list is empty or no element is found. This case must
> be checked before any use of the iterator, otherwise it will
> lead to a invalid memory access.
> 
> To fix this bug, use a new variable 'iter' as the list iterator,
> while use the origin variable 'ch' as a dedicated pointer to
> point to the found element.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1d8d80b4e4ff6 ("i40e: Add macvlan support on i40e")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> v2: Dropped patch "iavf: Fix error when changing ring parameters on ice PF"
> as its being reworked
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 27 +++++++++++----------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 6778df2177a1..98871f014994 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -7549,42 +7549,43 @@ static void i40e_free_macvlan_channels(struct i40e_vsi *vsi)
>  static int i40e_fwd_ring_up(struct i40e_vsi *vsi, struct net_device *vdev,
>  			    struct i40e_fwd_adapter *fwd)
>  {
> +	struct i40e_channel *ch = NULL, *ch_tmp, *iter;
>  	int ret = 0, num_tc = 1,  i, aq_err;
> -	struct i40e_channel *ch, *ch_tmp;
>  	struct i40e_pf *pf = vsi->back;
>  	struct i40e_hw *hw = &pf->hw;
>  
> -	if (list_empty(&vsi->macvlan_list))
> -		return -EINVAL;
> -
>  	/* Go through the list and find an available channel */
> -	list_for_each_entry_safe(ch, ch_tmp, &vsi->macvlan_list, list) {
> -		if (!i40e_is_channel_macvlan(ch)) {
> -			ch->fwd = fwd;
> +	list_for_each_entry_safe(iter, ch_tmp, &vsi->macvlan_list, list) {
> +		if (!i40e_is_channel_macvlan(iter)) {
> +			iter->fwd = fwd;
>  			/* record configuration for macvlan interface in vdev */
>  			for (i = 0; i < num_tc; i++)
>  				netdev_bind_sb_channel_queue(vsi->netdev, vdev,
>  							     i,
> -							     ch->num_queue_pairs,
> -							     ch->base_queue);
> -			for (i = 0; i < ch->num_queue_pairs; i++) {
> +							     iter->num_queue_pairs,
> +							     iter->base_queue);
> +			for (i = 0; i < iter->num_queue_pairs; i++) {
>  				struct i40e_ring *tx_ring, *rx_ring;
>  				u16 pf_q;
>  
> -				pf_q = ch->base_queue + i;
> +				pf_q = iter->base_queue + i;
>  
>  				/* Get to TX ring ptr */
>  				tx_ring = vsi->tx_rings[pf_q];
> -				tx_ring->ch = ch;
> +				tx_ring->ch = iter;
>  
>  				/* Get the RX ring ptr */
>  				rx_ring = vsi->rx_rings[pf_q];
> -				rx_ring->ch = ch;
> +				rx_ring->ch = iter;
>  			}
> +			ch = iter;
>  			break;

I guess this is some form of an intentional pattern so I won't delay
the fix. But Paolo pointed out in previous reviews of similar patches
that if the assignment to the old iterator name (ch in this case) was
done earlier in the function (right before iter->fwd = fwd;) we would
not have to modify so many lines of this function.

>  		}
>  	}
>  
> +	if (!ch)
> +		return -EINVAL;
> +
>  	/* Guarantee all rings are updated before we update the
>  	 * MAC address filter.
>  	 */