netdev - Re: [PATCH net] vrf: do not push non-ND strict packets with a source LLA through packet taps again

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Jun 2021 19:18:50 -0600
From:   David Ahern <dsahern@...il.com>
To:     Antoine Tenart <atenart@...nel.org>, davem@...emloft.net,
        kuba@...nel.org, dsahern@...nel.org
Cc:     netdev@...r.kernel.org, Stephen Suryaputra <ssuryaextr@...il.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] vrf: do not push non-ND strict packets with a source
 LLA through packet taps again

On 6/18/21 9:15 AM, Antoine Tenart wrote:
> Non-ND strict packets with a source LLA go through the packet taps
> again, while non-ND strict packets with other source addresses do not,
> and we can see a clone of those packets on the vrf interface (we should
> not). This is due to a series of changes:
> 
> Commit 6f12fa775530[1] made non-ND strict packets not being pushed again
> in the packet taps. This changed with commit 205704c618af[2] for those
> packets having a source LLA, as they need a lookup with the orig_iif.
> 
> The issue now is those packets do not skip the 'vrf_ip6_rcv' function to
> the end (as the ones without a source LLA) and go through the check to
> call packet taps again. This check was changed by commit 6f12fa775530[1]
> and do not exclude non-strict packets anymore. Packets matching
> 'need_strict && !is_ndisc && is_ll_src' are now being sent through the
> packet taps again. This can be seen by dumping packets on the vrf
> interface.
> 
> Fix this by having the same code path for all non-ND strict packets and
> selectively lookup with the orig_iif for those with a source LLA. This
> has the effect to revert to the pre-205704c618af[2] condition, which
> should also be easier to maintain.
> 
> [1] 6f12fa775530 ("vrf: mark skb for multicast or link-local as enslaved to VRF")
> [2] 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict")
> 
> Fixes: 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict")
> Cc: Stephen Suryaputra <ssuryaextr@...il.com>
> Reported-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> ---
>  drivers/net/vrf.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> index 28a6c4cfe9b8..414afcb0a23f 100644
> --- a/drivers/net/vrf.c
> +++ b/drivers/net/vrf.c
> @@ -1366,22 +1366,22 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev,
>  	int orig_iif = skb->skb_iif;
>  	bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr);
>  	bool is_ndisc = ipv6_ndisc_frame(skb);
> -	bool is_ll_src;
>  
>  	/* loopback, multicast & non-ND link-local traffic; do not push through
>  	 * packet taps again. Reset pkt_type for upper layers to process skb.
> -	 * for packets with lladdr src, however, skip so that the dst can be
> -	 * determine at input using original ifindex in the case that daddr
> -	 * needs strict
> +	 * For strict packets with a source LLA, determine the dst using the
> +	 * original ifindex.
>  	 */
> -	is_ll_src = ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL;
> -	if (skb->pkt_type == PACKET_LOOPBACK ||
> -	    (need_strict && !is_ndisc && !is_ll_src)) {
> +	if (skb->pkt_type == PACKET_LOOPBACK || (need_strict && !is_ndisc)) {
>  		skb->dev = vrf_dev;
>  		skb->skb_iif = vrf_dev->ifindex;
>  		IP6CB(skb)->flags |= IP6SKB_L3SLAVE;
> +
>  		if (skb->pkt_type == PACKET_LOOPBACK)
>  			skb->pkt_type = PACKET_HOST;
> +		else if (ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL)
> +			vrf_ip6_input_dst(skb, vrf_dev, orig_iif);
> +
>  		goto out;
>  	}
>  
> 

you are basically moving Stephen's is_ll_src within the need_strict and
not ND.

Did you run the fcnal-test script and verify no change in test results?