| 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
| ||
|
Date: Wed, 02 May 2012 16:10:39 -0700
From: Jay Vosburgh <fubar@...ibm.com>
To: Jiri Bohac <jbohac@...e.cz>
cc: Eric Dumazet <eric.dumazet@...il.com>,
Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org
Subject: Re: bonding: don't increase rx_dropped after processing LACPDUs
Jiri Bohac <jbohac@...e.cz> wrote:
>On Wed, May 02, 2012 at 10:36:49PM +0200, Eric Dumazet wrote:
>> > + if (ret == RX_HANDLER_CONSUMED)
>> > + kfree_skb(skb);
>>
>> After this point, you have use after free :
>>
>> if (bond_should_deliver_exact_match(skb, slave, bond)) {
>> ...
>> }
>> skb->dev = bond->dev;
>
>Thanks for spotting this! Let's just return immediately at that
>point. Fixed version below:
Won't this make it impossible to bind a PF_PACKET socket to
sll_protocol == ETH_P_SLOW and see the LACPDUs, but only when bonding is
running 802.3ad? This because the ptype_all check in
__netif_receive_skb happens before the rx_handler, but the ptype_base
check (bound packet socket, for example) happens after. Currently,
libpcap looks to bind to ETH_P_ALL, so it won't be affected.
If so, is that something we care about?
-J
>Signed-off-by: Jiri Bohac <jbohac@...e.cz>
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -2173,9 +2173,10 @@ re_arm:
> * received frames (loopback). Since only the payload is given to this
> * function, it check for loopback.
> */
>-static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
>+static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u16 length)
> {
> struct port *port;
>+ int ret = RX_HANDLER_ANOTHER;
>
> if (length >= sizeof(struct lacpdu)) {
>
>@@ -2184,11 +2185,12 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> if (!port->slave) {
> pr_warning("%s: Warning: port of slave %s is uninitialized\n",
> slave->dev->name, slave->dev->master->name);
>- return;
>+ return ret;
> }
>
> switch (lacpdu->subtype) {
> case AD_TYPE_LACPDU:
>+ ret = RX_HANDLER_CONSUMED;
> pr_debug("Received LACPDU on port %d\n",
> port->actor_port_number);
> /* Protect against concurrent state machines */
>@@ -2198,6 +2200,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> break;
>
> case AD_TYPE_MARKER:
>+ ret = RX_HANDLER_CONSUMED;
> // No need to convert fields to Little Endian since we don't use the marker's fields.
>
> switch (((struct bond_marker *)lacpdu)->tlv_type) {
>@@ -2219,6 +2222,7 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> }
> }
> }
>+ return ret;
> }
>
> /**
>@@ -2456,18 +2460,20 @@ out:
> return NETDEV_TX_OK;
> }
>
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave)
> {
>+ int ret = RX_HANDLER_ANOTHER;
> if (skb->protocol != PKT_TYPE_LACPDU)
>- return;
>+ return ret;
>
> if (!pskb_may_pull(skb, sizeof(struct lacpdu)))
>- return;
>+ return ret;
>
> read_lock(&bond->lock);
>- bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
>+ ret = bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
> read_unlock(&bond->lock);
>+ return ret;
> }
>
> /*
>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>index 235b2cc..5ee7e3c 100644
>--- a/drivers/net/bonding/bond_3ad.h
>+++ b/drivers/net/bonding/bond_3ad.h
>@@ -274,7 +274,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave);
> void bond_3ad_handle_link_change(struct slave *slave, char link);
> int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info);
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>-void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave);
> int bond_3ad_set_carrier(struct bonding *bond);
> void bond_3ad_update_lacp_rate(struct bonding *bond);
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 62d2409..0a0f4a6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1444,8 +1444,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *skb = *pskb;
> struct slave *slave;
> struct bonding *bond;
>- void (*recv_probe)(struct sk_buff *, struct bonding *,
>+ int (*recv_probe)(struct sk_buff *, struct bonding *,
> struct slave *);
>+ int ret = RX_HANDLER_ANOTHER;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (unlikely(!skb))
>@@ -1464,8 +1465,12 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>
> if (likely(nskb)) {
>- recv_probe(nskb, bond, slave);
>+ ret = recv_probe(nskb, bond, slave);
> dev_kfree_skb(nskb);
>+ if (ret == RX_HANDLER_CONSUMED) {
>+ kfree_skb(skb);
>+ return ret;
>+ }
> }
> }
>
>@@ -1487,7 +1492,7 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> memcpy(eth_hdr(skb)->h_dest, bond->dev->dev_addr, ETH_ALEN);
> }
>
>- return RX_HANDLER_ANOTHER;
>+ return ret;
> }
>
> /* enslave device <slave> to bond device <master> */
>@@ -2723,7 +2728,7 @@ static void bond_validate_arp(struct bonding *bond, struct slave *slave, __be32
> }
> }
>
>-static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>+static int bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> struct slave *slave)
> {
> struct arphdr *arp;
>@@ -2731,7 +2736,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
> __be32 sip, tip;
>
> if (skb->protocol != __cpu_to_be16(ETH_P_ARP))
>- return;
>+ return RX_HANDLER_ANOTHER;
>
> read_lock(&bond->lock);
>
>@@ -2776,6 +2781,7 @@ static void bond_arp_rcv(struct sk_buff *skb, struct bonding *bond,
>
> out_unlock:
> read_unlock(&bond->lock);
>+ return RX_HANDLER_ANOTHER;
> }
>
> /*
>
>
>--
>Jiri Bohac <jbohac@...e.cz>
>SUSE Labs, SUSE CZ
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
--
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