| 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: <tkrat.a8b022df2a5d588d@s5r6.in-berlin.de>
Date: Mon, 18 Jan 2010 22:36:49 +0100 (CET)
From: Stefan Richter <stefanr@...6.in-berlin.de>
To: Илья Басин <basinilya@...il.com>
cc: linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: [PATCH] firewire: net: fix panic in fwnet_write_complete
Date:
From: Stefan Richter <stefanr@...6.in-berlin.de>
Subject: firewire: net: fix panic in fwnet_write_complete
In the transmit path of firewire-net (IPv4 over 1394), the following
race condition may occur:
- The networking soft IRQ inserts a datagram into the 1394 async
request transmit DMA.
- The 1394 async transmit completion tasklet runs to finish cleaning
up (unlink datagram from list of pending ones, release skb and
outbound 1394 transaction object) --- before the networking soft IRQ
had a chance to proceed and add the datagram to the list of pending
datagrams.
This caused a panic in the 1394 async transmit completion tasklet when
it dereferenced unitialized list heads:
http://bugzilla.kernel.org/show_bug.cgi?id=15077
The fix is to add checks in the tx soft RQ and in the tasklet to
determine who of these two is the last referrer to the transaction
object. Then handle the cleanup of the object by the last referrer
rather than assuming that the tasklet is always the last one.
There is another similar race: Between said tasklet and fwnet_close,
i.e. at ifdown. However, that race is much less likely to occur in
practice and shall be fixed in a separate update.
Reported-by: Илья Басин <basinilya@...il.com>
Signed-off-by: Stefan Richter <stefanr@...6.in-berlin.de>
---
Илья, could you give this a try?
drivers/firewire/net.c | 53 ++++++++++++++++++++++++++++++-----------
1 file changed, 39 insertions(+), 14 deletions(-)
Index: linux-2.6.32.2/drivers/firewire/net.c
===================================================================
--- linux-2.6.32.2.orig/drivers/firewire/net.c
+++ linux-2.6.32.2/drivers/firewire/net.c
@@ -893,20 +893,31 @@ static void fwnet_receive_broadcast(stru
static struct kmem_cache *fwnet_packet_task_cache;
+static void fwnet_free_ptask(struct fwnet_packet_task *ptask)
+{
+ dev_kfree_skb_any(ptask->skb);
+ kmem_cache_free(fwnet_packet_task_cache, ptask);
+}
+
static int fwnet_send_packet(struct fwnet_packet_task *ptask);
static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask)
{
- struct fwnet_device *dev;
+ struct fwnet_device *dev = ptask->dev;
unsigned long flags;
-
- dev = ptask->dev;
+ bool free;
spin_lock_irqsave(&dev->lock, flags);
- list_del(&ptask->pt_link);
- spin_unlock_irqrestore(&dev->lock, flags);
- ptask->outstanding_pkts--; /* FIXME access inside lock */
+ ptask->outstanding_pkts--;
+
+ /* Check whether we or the networking TX soft-IRQ is last user. */
+ free = (ptask->outstanding_pkts == 0 && !list_empty(&ptask->pt_link));
+
+ if (ptask->outstanding_pkts == 0)
+ list_del(&ptask->pt_link);
+
+ spin_unlock_irqrestore(&dev->lock, flags);
if (ptask->outstanding_pkts > 0) {
u16 dg_size;
@@ -951,10 +962,10 @@ static void fwnet_transmit_packet_done(s
ptask->max_payload = skb->len + RFC2374_FRAG_HDR_SIZE;
}
fwnet_send_packet(ptask);
- } else {
- dev_kfree_skb_any(ptask->skb);
- kmem_cache_free(fwnet_packet_task_cache, ptask);
}
+
+ if (free)
+ fwnet_free_ptask(ptask);
}
static void fwnet_write_complete(struct fw_card *card, int rcode,
@@ -977,6 +988,7 @@ static int fwnet_send_packet(struct fwne
unsigned tx_len;
struct rfc2734_header *bufhdr;
unsigned long flags;
+ bool free;
dev = ptask->dev;
tx_len = ptask->max_payload;
@@ -1022,12 +1034,16 @@ static int fwnet_send_packet(struct fwne
generation, SCODE_100, 0ULL, ptask->skb->data,
tx_len + 8, fwnet_write_complete, ptask);
- /* FIXME race? */
spin_lock_irqsave(&dev->lock, flags);
- list_add_tail(&ptask->pt_link, &dev->broadcasted_list);
+
+ /* If the AT tasklet already ran, we may be last user. */
+ free = (ptask->outstanding_pkts == 0 && list_empty(&ptask->pt_link));
+ if (!free)
+ list_add_tail(&ptask->pt_link, &dev->broadcasted_list);
+
spin_unlock_irqrestore(&dev->lock, flags);
- return 0;
+ goto out;
}
fw_send_request(dev->card, &ptask->transaction,
@@ -1035,12 +1051,19 @@ static int fwnet_send_packet(struct fwne
ptask->generation, ptask->speed, ptask->fifo_addr,
ptask->skb->data, tx_len, fwnet_write_complete, ptask);
- /* FIXME race? */
spin_lock_irqsave(&dev->lock, flags);
- list_add_tail(&ptask->pt_link, &dev->sent_list);
+
+ /* If the AT tasklet already ran, we may be last user. */
+ free = (ptask->outstanding_pkts == 0 && list_empty(&ptask->pt_link));
+ if (!free)
+ list_add_tail(&ptask->pt_link, &dev->sent_list);
+
spin_unlock_irqrestore(&dev->lock, flags);
dev->netdev->trans_start = jiffies;
+ out:
+ if (free)
+ fwnet_free_ptask(ptask);
return 0;
}
@@ -1298,6 +1321,8 @@ static netdev_tx_t fwnet_tx(struct sk_bu
spin_unlock_irqrestore(&dev->lock, flags);
ptask->max_payload = max_payload;
+ INIT_LIST_HEAD(&ptask->pt_link);
+
fwnet_send_packet(ptask);
return NETDEV_TX_OK;
--
Stefan Richter
-=====-==-=- ---= =--=-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists