netdev - Re: [PATCH net-next 8/9] net: ipv6: add skb drop reasons to ip6_rcv

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] [day] [month] [year] [list]
Message-ID: <CADxym3Z+Ginefiw7ymbZFGa3QC5qaT5jM3DjedijzGwYoFqPaw@mail.gmail.com>
Date:   Thu, 14 Apr 2022 09:20:21 +0800
From:   Menglong Dong <menglong8.dong@...il.com>
To:     Eric Dumazet <edumazet@...il.com>
Cc:     David Ahern <dsahern@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Biao Jiang <benbjiang@...cent.com>,
        Hao Peng <flyingpeng@...cent.com>,
        Menglong Dong <imagedong@...cent.com>,
        Eric Dumazet <edumazet@...gle.com>, Martin Lau <kafai@...com>,
        Talal Ahmad <talalahmad@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Mengen Sun <mengensun@...cent.com>, dongli.zhang@...cle.com,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 8/9] net: ipv6: add skb drop reasons to ip6_rcv_core()

On Thu, Apr 14, 2022 at 4:41 AM Eric Dumazet <edumazet@...il.com> wrote:
>
>
> On 4/13/22 01:15, menglong8.dong@...il.com wrote:
> > From: Menglong Dong <imagedong@...cent.com>
> >
> > Replace kfree_skb() used in ip6_rcv_core() with kfree_skb_reason().
> > No new drop reasons are added.
> >
> > Seems now we use 'SKB_DROP_REASON_IP_INHDR' for too many case during
> > ipv6 header parse or check, just like what 'IPSTATS_MIB_INHDRERRORS'
> > do. Will it be too general and hard to know what happened?
> >
> > Signed-off-by: Menglong Dong <imagedong@...cent.com>
> > Reviewed-by: Jiang Biao <benbjiang@...cent.com>
> > Reviewed-by: Hao Peng <flyingpeng@...cent.com>
> > ---
> >   net/ipv6/ip6_input.c | 24 ++++++++++++++++--------
> >   1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> > index b4880c7c84eb..1b925ecb26e9 100644
> > --- a/net/ipv6/ip6_input.c
> > +++ b/net/ipv6/ip6_input.c
> > @@ -145,13 +145,14 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
> >   static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >                                   struct net *net)
> >   {
> > +     enum skb_drop_reason reason;
> >       const struct ipv6hdr *hdr;
> >       u32 pkt_len;
> >       struct inet6_dev *idev;
> >
> >       if (skb->pkt_type == PACKET_OTHERHOST) {
> >               dev_core_stats_rx_otherhost_dropped_inc(skb->dev);
> > -             kfree_skb(skb);
> > +             kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST);
> >               return NULL;
> >       }
> >
> > @@ -161,9 +162,12 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >
> >       __IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_IN, skb->len);
> >
> > +     SKB_DR_SET(reason, NOT_SPECIFIED);
> >       if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL ||
> >           !idev || unlikely(idev->cnf.disable_ipv6)) {
> >               __IP6_INC_STATS(net, idev, IPSTATS_MIB_INDISCARDS);
> > +             if (unlikely(idev->cnf.disable_ipv6))
>
> idev can be NULL here (according to surrounding code), and we crash :/

Yeah, you are right :)

Thanks for your fix!

>
>
>
> > +                     SKB_DR_SET(reason, IPV6DISABLED);
> >               goto drop;
> >       }
> >
> > @@ -187,8 +191,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >
> >       hdr = ipv6_hdr(skb);
> >
> > -     if (hdr->version != 6)
> > +     if (hdr->version != 6) {
> > +             SKB_DR_SET(reason, UNHANDLED_PROTO);
> >               goto err;
> > +     }
> >
> >       __IP6_ADD_STATS(net, idev,
> >                       IPSTATS_MIB_NOECTPKTS +
> > @@ -226,8 +232,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       if (!ipv6_addr_is_multicast(&hdr->daddr) &&
> >           (skb->pkt_type == PACKET_BROADCAST ||
> >            skb->pkt_type == PACKET_MULTICAST) &&
> > -         idev->cnf.drop_unicast_in_l2_multicast)
> > +         idev->cnf.drop_unicast_in_l2_multicast) {
> > +             SKB_DR_SET(reason, UNICAST_IN_L2_MULTICAST);
> >               goto err;
> > +     }
> >
> >       /* RFC4291 2.7
> >        * Nodes must not originate a packet to a multicast address whose scope
> > @@ -256,12 +264,11 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >               if (pkt_len + sizeof(struct ipv6hdr) > skb->len) {
> >                       __IP6_INC_STATS(net,
> >                                       idev, IPSTATS_MIB_INTRUNCATEDPKTS);
> > +                     SKB_DR_SET(reason, PKT_TOO_SMALL);
> >                       goto drop;
> >               }
> > -             if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) {
> > -                     __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > -                     goto drop;
> > -             }
> > +             if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr)))
> > +                     goto err;
> >               hdr = ipv6_hdr(skb);
> >       }
> >
> > @@ -282,9 +289,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       return skb;
> >   err:
> >       __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > +     SKB_DR_OR(reason, IP_INHDR);
> >   drop:
> >       rcu_read_unlock();
> > -     kfree_skb(skb);
> > +     kfree_skb_reason(skb, reason);
> >       return NULL;
> >   }
> >