netdev - Re: [PATCH net-next v15] openvswitch: enable NSH support

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: <20171106132248.6fa4c5a0@redhat.com>
Date:   Mon, 6 Nov 2017 13:22:48 +0100
From:   Jiri Benc <jbenc@...hat.com>
To:     Pravin Shelar <pshelar@....org>
Cc:     Yi Yang <yi.y.yang@...el.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        ovs dev <dev@...nvswitch.org>, Eric Garver <e@...g.me>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support

On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar wrote:
> > +int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh)
> > +{
> > +       struct nshhdr *nh;
> > +       size_t length = nsh_hdr_len(pushed_nh);
> > +       u8 next_proto;
> > +
> > +       if (skb->mac_len) {
> > +               next_proto = TUN_P_ETHERNET;
> > +       } else {
> > +               next_proto = tun_p_from_eth_p(skb->protocol);
> > +               if (!next_proto)
> > +                       return -EAFNOSUPPORT;  
> check for supported protocols can be moved to flow install validation
> in __ovs_nla_copy_actions().

You mean the check for !next_proto? It needs to be present for
correctness of nsh_push. This function has to be self contained, it
will be used by more callers than opevswitch, namely tc.

It's actually not so much a check for "supported protocols", it's
rather a check of return value of a function that converts ethertype to
a 1 byte tunnel type. Blindly using a result of a function that may
return error would be wrong. Openvswitch is free to perform additional
checks but this needs to stay.

> > +int nsh_pop(struct sk_buff *skb)
> > +{
> > +       struct nshhdr *nh;
> > +       size_t length;
> > +       __be16 inner_proto;
> > +
> > +       if (!pskb_may_pull(skb, NSH_BASE_HDR_LEN))
> > +               return -ENOMEM;
> > +       nh = (struct nshhdr *)(skb->data);
> > +       length = nsh_hdr_len(nh);
> > +       inner_proto = tun_p_to_eth_p(nh->np);  
> same as above, this check can be moved to flow install __ovs_nla_copy_actions().

There's no check here.

> > +       if (!pskb_may_pull(skb, length))
> > +               return -ENOMEM;
> > +
> > +       if (!inner_proto)
> > +               return -EAFNOSUPPORT;

Did you mean this one instead? Then see above, this has to stay.

 Jiri