netdev - Re: [PATCH net-next v2] openvswitch: add TTL decrement action

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, 20 Dec 2019 13:36:59 +0100
From:   Matteo Croce <mcroce@...hat.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     netdev <netdev@...r.kernel.org>, ovs dev <dev@...nvswitch.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Pravin B Shelar <pshelar@....org>,
        "David S. Miller" <davem@...emloft.net>,
        Bindiya Kurle <bindiyakurle@...il.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Ben Pfaff <blp@....org>
Subject: Re: [PATCH net-next v2] openvswitch: add TTL decrement action

On Tue, Dec 17, 2019 at 5:30 PM Nikolay Aleksandrov
<nikolay@...ulusnetworks.com> wrote:
>
> On 17/12/2019 17:51, Matteo Croce wrote:
> > New action to decrement TTL instead of setting it to a fixed value.
> > This action will decrement the TTL and, in case of expired TTL, drop it
> > or execute an action passed via a nested attribute.
> > The default TTL expired action is to drop the packet.
> >
> > Supports both IPv4 and IPv6 via the ttl and hop_limit fields, respectively.
> >
> > Tested with a corresponding change in the userspace:
> >
> >     # ovs-dpctl dump-flows
> >     in_port(2),eth(),eth_type(0x0800), packets:0, bytes:0, used:never, actions:dec_ttl{ttl<=1 action:(drop)},1,1
> >     in_port(1),eth(),eth_type(0x0800), packets:0, bytes:0, used:never, actions:dec_ttl{ttl<=1 action:(drop)},1,2
> >     in_port(1),eth(),eth_type(0x0806), packets:0, bytes:0, used:never, actions:2
> >     in_port(2),eth(),eth_type(0x0806), packets:0, bytes:0, used:never, actions:1
> >
> >     # ping -c1 192.168.0.2 -t 42
> >     IP (tos 0x0, ttl 41, id 61647, offset 0, flags [DF], proto ICMP (1), length 84)
> >         192.168.0.1 > 192.168.0.2: ICMP echo request, id 386, seq 1, length 64
> >     # ping -c1 192.168.0.2 -t 120
> >     IP (tos 0x0, ttl 119, id 62070, offset 0, flags [DF], proto ICMP (1), length 84)
> >         192.168.0.1 > 192.168.0.2: ICMP echo request, id 388, seq 1, length 64
> >     # ping -c1 192.168.0.2 -t 1
> >     #
> >
> > Co-authored-by: Bindiya Kurle <bindiyakurle@...il.com>
> > Signed-off-by: Bindiya Kurle <bindiyakurle@...il.com>
> > Signed-off-by: Matteo Croce <mcroce@...hat.com>
> > ---
> >  include/uapi/linux/openvswitch.h |  22 +++++++
> >  net/openvswitch/actions.c        |  71 +++++++++++++++++++++
> >  net/openvswitch/flow_netlink.c   | 105 +++++++++++++++++++++++++++++++
> >  3 files changed, 198 insertions(+)
> >
>
> Hi Matteo,
>
> [snip]
> > +}
> > +
> >  /* When 'last' is true, sample() should always consume the 'skb'.
> >   * Otherwise, sample() should keep 'skb' intact regardless what
> >   * actions are executed within sample().
> > @@ -1176,6 +1201,44 @@ static int execute_check_pkt_len(struct datapath *dp, struct sk_buff *skb,
> >                            nla_len(actions), last, clone_flow_key);
> >  }
> >
> > +static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > +     int err;
> > +
> > +     if (skb->protocol == htons(ETH_P_IPV6)) {
> > +             struct ipv6hdr *nh = ipv6_hdr(skb);
> > +
> > +             err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > +                                       sizeof(*nh));
>
> skb_ensure_writable() calls pskb_may_pull() which may reallocate so nh might become invalid.
> It seems the IPv4 version below is ok as the ptr is reloaded.
>

Right

> One q as I don't know ovs that much - can this action be called only with
> skb->protocol ==  ETH_P_IP/IPV6 ? I.e. Are we sure that if it's not v6, then it must be v4 ?
>

I'm adding a check in validate_and_copy_dec_ttl() so only ipv4/ipv6
packet will pass.

Thanks,

-- 
Matteo Croce
per aspera ad upstream