| 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: <CALnjE+rL5MKW=8X0QC5taPh_asJXZCG8XPgU=8_hNbp4-C8yYw@mail.gmail.com>
Date: Sat, 20 Jul 2013 23:08:01 -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 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.
>
>
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?
--
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