netdev - Re: [PATCH] skbuff: ensure to reset dev in skb_scrub

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: <CALnjE+pPUo8zvDHPKKd9mD-jKvgUd=krkt4UnFF06rKVsHPvgg@mail.gmail.com>
Date:	Fri, 19 Jul 2013 14:50:21 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	nicolas.dichtel@...nd.com
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] skbuff: ensure to reset dev in skb_scrub_packet()

On Fri, Jul 19, 2013 at 1:40 PM, Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
> Le 19/07/2013 20:21, Pravin Shelar a écrit :
>
>> On Fri, Jul 19, 2013 at 7:41 AM, Nicolas Dichtel
>> <nicolas.dichtel@...nd.com> wrote:
>>>
>>> Because this function is used to scrub a packet when it cross netns, we
>>> must
>>> ensure that skb->dev points to the new netns.
>>>
>>> This was done by eth_type_trans() in dev_forward_skb(), but it's also
>>> needed
>>> for ip tunnels.
>>>
>>> I take the opportunity to move the call of skb_scrub_packet() after
>>> eth_type_trans(), to be sure that pkt_type is set to PACKET_HOST.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
>>> ---
>>>   include/linux/skbuff.h | 3 ++-
>>>   net/core/dev.c         | 6 +++---
>>>   net/core/skbuff.c      | 3 ++-
>>>   net/ipv4/ip_tunnel.c   | 9 +++++----
>>>   net/ipv6/sit.c         | 4 ++--
>>>   5 files changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 5afefa01a13c..620ecce0a717 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -2385,7 +2385,8 @@ extern void              skb_split(struct sk_buff
>>> *skb,
>>>                                   struct sk_buff *skb1, const u32 len);
>>>   extern int            skb_shift(struct sk_buff *tgt, struct sk_buff
>>> *skb,
>>>                                   int shiftlen);
>>> -extern void           skb_scrub_packet(struct sk_buff *skb);
>>> +extern void           skb_scrub_packet(struct sk_buff *skb,
>>> +                                       struct net_device *dev);
>>>
>>>   extern struct sk_buff *skb_segment(struct sk_buff *skb,
>>>                                     netdev_features_t features);
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 26755dd40daa..6f789b99331b 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -1691,13 +1691,13 @@ int dev_forward_skb(struct net_device *dev,
>>> struct sk_buff *skb)
>>>                  kfree_skb(skb);
>>>                  return NET_RX_DROP;
>>>          }
>>> -       skb_scrub_packet(skb);
>>>          skb->protocol = eth_type_trans(skb, dev);
>>>
>>>          /* eth_type_trans() can set pkt_type.
>>> -        * clear pkt_type _after_ calling eth_type_trans()
>>> +        * call skb_scrub_packet() after it to clear pkt_type _after_
>>> calling
>>> +        * eth_type_trans().
>>>           */
>>> -       skb->pkt_type = PACKET_HOST;
>>> +       skb_scrub_packet(skb, dev);
>>>
>>>          return netif_rx(skb);
>>>   }
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index 20e02d2605ec..5f4701f89af8 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -3507,13 +3507,14 @@ EXPORT_SYMBOL(skb_try_coalesce);
>>>    * another namespace. We have to clear all information in the skb that
>>>    * could impact namespace isolation.
>>>    */
>>> -void skb_scrub_packet(struct sk_buff *skb)
>>> +void skb_scrub_packet(struct sk_buff *skb, struct net_device *dev)
>>>   {
>>>          skb_orphan(skb);
>>>          skb->tstamp.tv64 = 0;
>>>          skb->pkt_type = PACKET_HOST;
>>>          skb->skb_iif = 0;
>>>          skb_dst_drop(skb);
>>> +       skb->dev = dev;
>>>          skb->mark = 0;
>>>          secpath_reset(skb);
>>>          nf_reset(skb);
>>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>>> index ca1cb2d5f6e2..2e88321c7f23 100644
>>> --- a/net/ipv4/ip_tunnel.c
>>> +++ b/net/ipv4/ip_tunnel.c
>>> @@ -454,15 +454,16 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct
>>> sk_buff *skb,
>>>          tstats->rx_bytes += skb->len;
>>>          u64_stats_update_end(&tstats->syncp);
>>>
>>> -       if (tunnel->net != dev_net(tunnel->dev))
>>> -               skb_scrub_packet(skb);
>>> -
>>>          if (tunnel->dev->type == ARPHRD_ETHER) {
>>>                  skb->protocol = eth_type_trans(skb, tunnel->dev);
>>>                  skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>>>          } else {
>>>                  skb->dev = tunnel->dev;
>>>          }
>>> +
>>> +       if (tunnel->net != dev_net(tunnel->dev))
>>> +               skb_scrub_packet(skb, tunnel->dev);
>>> +
>>
>>
>> It is done in ip_tunnels right above the statement. Where exactly are
>> we missing skb->dev set to tunnel->dev?
>
> On the xmit path, ipip6_tunnel_xmit() for example.

This functions calls iptunnel_xmit(), which will finally calls
ip_output() which does same assignment for every case. How is that
different than assigning it in skb_scrub_packet()?

>
> And note also, that skb_scrub_packet() is used for netns crossing, hence
> this function should be complete and must not leave some field with pointer
> to the previous netns.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html