| 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: <51ED99FE.8080605@6wind.com>
Date: Mon, 22 Jul 2013 22:45:50 +0200
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Pravin Shelar <pshelar@...ira.com>
CC: davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] skbuff: ensure to reset dev in skb_scrub_packet()
Le 21/07/2013 08:08, Pravin Shelar a écrit :
> On Sat, Jul 20, 2013 at 1:26 PM, Nicolas Dichtel
> <nicolas.dichtel@...nd.com> wrote:
>> Le 19/07/2013 23:50, Pravin Shelar a écrit :
>>
>>> 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()?
>>
>> Ok, I miss it. But my next comment still applies.
xfrm4_output() does not set skb->dev, you may enter netfilter engine with a
skb->dev pointing to the previous netns.
>>
>>
> ok, Let me try again.
>
>>>
>>>>
>>>> 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.
>
> why do you want to add redundant assignments?
Because I think it's wrong to wait more time to 'scrub' this field. The netns
has changed, but skb is still pointing to the previous one.
Before calling dst_output(), skb enter the netfilter engine.
--
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
Powered by blists - more mailing lists