netdev - Re: [PATCH net-next] vrf: plug skb leaks

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-next>] [day] [month] [year] [list]

Message-Id: <B23BDD39-42FF-49D6-8515-680F9D5F3970@cumulusnetworks.com>
Date:	Wed, 19 Aug 2015 23:17:27 +0300
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	David Ahern <dsa@...ulusnetworks.com>
Cc:	David Miller <davem@...emloft.net>, shm@...ulusnetworks.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] vrf: plug skb leaks


> On Aug 19, 2015, at 8:27 PM, Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:
> 
> 
> On Aug 19, 2015 20:13, "David Ahern" <dsa@...ulusnetworks.com> wrote:
> >
> > Hi Nikolay:
> >
> >
> > On 8/18/15 8:12 PM, Nikolay Aleksandrov wrote:
> >>
> >> diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
> >> index ed208317cbb5..4aa06450fafa 100644
> >> --- a/drivers/net/vrf.c
> >> +++ b/drivers/net/vrf.c
> >> @@ -97,6 +97,12 @@ static bool is_ip_rx_frame(struct sk_buff *skb)
> >>         return false;
> >>   }
> >>
> >> +static void vrf_tx_error(struct net_device *vrf_dev, struct sk_buff *skb)
> >> +{
> >> +       vrf_dev->stats.tx_errors++;
> >> +       kfree_skb(skb);
> >> +}
> >> +
> >>   /* note: already called with rcu_read_lock */
> >>   static rx_handler_result_t vrf_handle_frame(struct sk_buff **pskb)
> >>   {
> >> @@ -149,7 +155,8 @@ static struct rtnl_link_stats64 *vrf_get_stats64(struct net_device *dev,
> >>   static netdev_tx_t vrf_process_v6_outbound(struct sk_buff *skb,
> >>                                            struct net_device *dev)
> >>   {
> >> -       return 0;
> >> +       vrf_tx_error(dev, skb);
> >> +       return NET_XMIT_DROP;
> >>   }
> >>
> >>   static int vrf_send_v4_prep(struct sk_buff *skb, struct flowi4 *fl4,
> >> @@ -206,8 +213,7 @@ static netdev_tx_t vrf_process_v4_outbound(struct sk_buff *skb,
> >>   out:
> >>         return ret;
> >>   err:
> >> -       vrf_dev->stats.tx_errors++;
> >> -       kfree_skb(skb);
> >> +       vrf_tx_error(vrf_dev, skb);
> >>         goto out;
> >>   }
> >>
> >> @@ -219,6 +225,7 @@ static netdev_tx_t is_ip_tx_frame(struct sk_buff *skb, struct net_device *dev)
> >>         case htons(ETH_P_IPV6):
> >>                 return vrf_process_v6_outbound(skb, dev);
> >>         default:
> >> +               vrf_tx_error(dev, skb);
> >>                 return NET_XMIT_DROP;
> >>         }
> >>   }
> >>
> >
> > Would be simpler to do the vrf_tx_error at the end of is_ip_tx_frame() if ret == NET_XMIT_DROP.
> >
> > David
> >
> 
> Sure, that will work too.
> 

Actually no, this will not work because ip_local_out() can return NET_XMIT_DROP and the packet
can already be dropped. I’d prefer to keep these cases separate and explicit as they are in my patch.


--
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