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, 19 Jul 2013 11:21:52 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Nicolas Dichtel <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 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?


>         gro_cells_receive(&tunnel->gro_cells, skb);
>         return 0;
>
> @@ -614,7 +615,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
>         }
>
>         if (tunnel->net != dev_net(dev))
> -               skb_scrub_packet(skb);
> +               skb_scrub_packet(skb, rt->dst.dev);
>
>         if (tunnel->err_count > 0) {
>                 if (time_before(jiffies,
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index a3437a4cd07e..dc2d01f90b81 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -622,7 +622,7 @@ static int ipip6_rcv(struct sk_buff *skb)
>                 tstats->rx_bytes += skb->len;
>
>                 if (tunnel->net != dev_net(tunnel->dev))
> -                       skb_scrub_packet(skb);
> +                       skb_scrub_packet(skb, tunnel->dev);
>                 netif_rx(skb);
>
>                 return 0;
> @@ -861,7 +861,7 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb,
>         }
>
>         if (tunnel->net != dev_net(dev))
> -               skb_scrub_packet(skb);
> +               skb_scrub_packet(skb, tdev);
>
>         /*
>          * Okay, now see if we can stuff it in the buffer as-is.
> --
> 1.8.2.1
>
> --
> 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
--
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