linux-kernel - Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan

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]

Message-ID: <ZsSLj-fI5Hiy9aV-@shredder.mtl.com>
Date: Tue, 20 Aug 2024 15:26:55 +0300
From: Ido Schimmel <idosch@...dia.com>
To: Menglong Dong <menglong8.dong@...il.com>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, dsahern@...nel.org, dongml2@...natelecom.cn,
	amcohen@...dia.com, gnault@...hat.com, bpoirier@...dia.com,
	b.galvani@...il.com, razor@...ckwall.org, petrm@...dia.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to
 vxlan_rcv()

On Thu, Aug 15, 2024 at 08:42:58PM +0800, Menglong Dong wrote:
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index e971c4785962..9a61f04bb95d 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
>  /* Callback from net/ipv4/udp.c to receive packets */
>  static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  {
> +	enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
>  	struct vxlan_vni_node *vninode = NULL;
>  	struct vxlan_dev *vxlan;
>  	struct vxlan_sock *vs;
> @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  	int nh;
>  
>  	/* Need UDP and VXLAN header to be present */
> -	if (!pskb_may_pull(skb, VXLAN_HLEN))
> +	if (reason != SKB_NOT_DROPPED_YET)
>  		goto drop;
>  
>  	unparsed = *vxlan_hdr(skb);
> @@ -1690,6 +1691,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  		netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
>  			   ntohl(vxlan_hdr(skb)->vx_flags),
>  			   ntohl(vxlan_hdr(skb)->vx_vni));
> +		reason = (u32)VXLAN_DROP_FLAGS;

I don't find "FLAGS" very descriptive. AFAICT the reason is used in
these two cases:

1. "I flag" is not set
2. The reserved fields are not zero

Maybe call it VXLAN_DROP_INVALID_HDR ?

And I agree with the comment about documenting these drop reasons like
in include/net/dropreason-core.h

>  		/* Return non vxlan pkt */
>  		goto drop;
>  	}
> @@ -1703,8 +1705,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  	vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
>  
>  	vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode);
> -	if (!vxlan)
> +	if (!vxlan) {
> +		reason = (u32)VXLAN_DROP_VNI;

Same comment here. Maybe VXLAN_DROP_VNI_NOT_FOUND ?

>  		goto drop;
> +	}
>  
>  	/* For backwards compatibility, only allow reserved fields to be
>  	 * used by VXLAN extensions if explicitly requested.
> @@ -1717,12 +1721,16 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  	}
>  
>  	if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto,
> -				   !net_eq(vxlan->net, dev_net(vxlan->dev))))
> +				   !net_eq(vxlan->net, dev_net(vxlan->dev)))) {
> +		reason = SKB_DROP_REASON_NOMEM;
>  		goto drop;
> +	}
>  
> -	if (vs->flags & VXLAN_F_REMCSUM_RX)
> -		if (unlikely(!vxlan_remcsum(&unparsed, skb, vs->flags)))
> +	if (vs->flags & VXLAN_F_REMCSUM_RX) {
> +		reason = vxlan_remcsum(&unparsed, skb, vs->flags);
> +		if (unlikely(reason != SKB_NOT_DROPPED_YET))
>  			goto drop;
> +	}
>  
>  	if (vxlan_collect_metadata(vs)) {
>  		IP_TUNNEL_DECLARE_FLAGS(flags) = { };
> @@ -1732,8 +1740,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  		tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), flags,
>  					 key32_to_tunnel_id(vni), sizeof(*md));
>  
> -		if (!tun_dst)
> +		if (!tun_dst) {
> +			reason = SKB_DROP_REASON_NOMEM;
>  			goto drop;
> +		}
>  
>  		md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
>  
> @@ -1757,12 +1767,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  		 * is more robust and provides a little more security in
>  		 * adding extensions to VXLAN.
>  		 */
> +		reason = (u32)VXLAN_DROP_FLAGS;
>  		goto drop;
>  	}
>  
>  	if (!raw_proto) {
> -		if (!vxlan_set_mac(vxlan, vs, skb, vni))
> +		if (!vxlan_set_mac(vxlan, vs, skb, vni)) {
> +			reason = (u32)VXLAN_DROP_MAC;

The function drops the packet for various reasons:

1. Source MAC is equal to the VXLAN device's MAC
2. Source MAC is invalid (all zeroes or multicast)
3. Trying to migrate a static entry or one pointing to a nexthop

They are all quite obscure so it might be fine to fold them under the
same reason, but I can't find a descriptive name.

If you split 1-2 to one reason and 3 to another, then they can become
VXLAN_DROP_INVALID_SMAC and VXLAN_DROP_ENTRY_EXISTS

>  			goto drop;
> +		}