| 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
| ||
|
Message-ID: <CADxym3YK4_8VCYgzsW_o0Cc3CXZMV8tUVb1Mq2RhH4=VoecPLQ@mail.gmail.com>
Date: Wed, 21 Aug 2024 20:54:05 +0800
From: Menglong Dong <menglong8.dong@...il.com>
To: Ido Schimmel <idosch@...dia.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 Tue, Aug 20, 2024 at 8:27 PM Ido Schimmel <idosch@...dia.com> wrote:
>
> 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 ?
>
Yeah, that makes sense.
> And I agree with the comment about documenting these drop reasons like
> in include/net/dropreason-core.h
>
Okay, I'm planning to do it this way.
> > /* 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 ?
>
Yeah, sounds nice.
> > 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
>
Sounds great! Thanks for creating these names for me,
I'm really not good at naming :/
> > goto drop;
> > + }
Powered by blists - more mailing lists