[PATCH] firewire: fix panic in handle_at_packet

[PATCH] firewire: fix panic in handle_at_packet

[Stefan Richter]

This fixes a use-after-free bug in the handling of split transactions.
The AT DMA handler of the request was occasionally executed after the
AR DMA handler of the response.  The AT DMA handler then accessed an
already freed packet.

Reported by Johannes Berg <johannes@sipsolutions.net>.
http://bugzilla.kernel.org/show_bug.cgi?id=9617

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-transaction.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux/drivers/firewire/fw-transaction.c
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.c
+++ linux/drivers/firewire/fw-transaction.c
@@ -737,6 +737,12 @@ fw_core_handle_response(struct fw_card *
 		break;
 	}
 
+	/*
+	 * The response handler may be executed while the request handler
+	 * is still pending.  Cancel the request handler.
+	 */
+	card->driver->cancel_packet(card, &t->packet);
+
 	t->callback(card, rcode, data, data_length, t->callback_data);
 }
 EXPORT_SYMBOL(fw_core_handle_response);

-- 
Stefan Richter
-=====-==--- --== =----
http://arcgraph.de/sr/

Re: [PATCH] firewire: fix panic in handle_at_packet

[Jarod Wilson]

On Saturday 15 March 2008 07:56:41 pm Stefan Richter wrote:
> This fixes a use-after-free bug in the handling of split transactions.
> The AT DMA handler of the request was occasionally executed after the
> AR DMA handler of the response.  The AT DMA handler then accessed an
> already freed packet.
>
> Reported by Johannes Berg <johannes@sipsolutions.net>.
> http://bugzilla.kernel.org/show_bug.cgi?id=9617
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Panics hooking up to an x86 mac mini in target disk mode are gone on my end 
with this patch added, and the fix makes sense -- assuming I've got it right, 
in my head, of course. ;)

As I understand it, we'll now simply bail in handle_at_packet when we see 
packet == NULL, rather than trying to play with already freed memory, and 
cancelling an AT packet here should always be perfectly safe, because we're 
already onto the AR side of this transaction, and in most cases, the AT 
handler already fired anyway.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH] firewire: fix panic in handle_at_packet

[Stefan Richter]

Jarod Wilson wrote:
> Panics hooking up to an x86 mac mini in target disk mode are gone on my end 
> with this patch added, and the fix makes sense -- assuming I've got it right, 
> in my head, of course. ;)
> 
> As I understand it, we'll now simply bail in handle_at_packet when we see 
> packet == NULL, rather than trying to play with already freed memory, and 
> cancelling an AT packet here should always be perfectly safe, because we're 
> already onto the AR side of this transaction, and in most cases, the AT 
> handler already fired anyway.

To expand on this a little bit:

An outbound transaction goes like this (possible retries and failures 
neglected):
   - fw-core submits a request packet to fw-ohci.
   - fw-ohci creates the DMA program and hands it over to the controller.
   - Controller sends the request packet to the remote node.
   - Remote node sends an ACK.
   - Controller generates an interrupt.
   - fw-ohci schedules the AT handler tasklet.
   - If this was a so-called unified transaction, the transaction is now
     complete.  The AT handler tasklet will eventually call into fw-core
     to let it finish the transaction.
   - If this was a so-called split transaction, the remote node will send
     a response packet.
   - Controller ACKs it and generates an interrupt.
   - fw-ohci schedules the AR handler tasklet.
   - The previously scheduled AT handler tasklet, *if* it is actually
     executed for the sent and ACKed request packet *before* the AR
     handler tasklet for the received response packet, will call into
     fw-core which simply sets a timestamp for the request packet.  (This
     timestamp should be used as the basis for split transaction timeout,
     but fw-core doesn't really look at that timestamp yet.  We discussed
     this recently.)
   - The scheduled AR handler calls into fw-core which completes the
     transaction.

We schedule the request AT handler tasklet before the response AR 
handler tasklet, but under some circumstances the kernel may decide to 
defer execution of the former tasklet but to immediately execute the 
latter tasklet.  That's why they can be reordered.

This reordering is, from fw-core's point of view, an implementation 
detail of fw-ohci and fw-core shouldn't be bothered with this.  However, 
fw-ohci is unable to figure out which responses belong to which 
requests.  (Matching requests and respnses is exclusively fw-core's 
transaction layer's job.)  Hence fw-ohci needs to be told by the 
transaction layer about which requests don't need any handling anymore 
because their transaction was already completed.

And yes, the call to cancel_packet is not only necessary, it is also safe:
   - We call it only for request packets whose response we already
     received.
   - There is nothing useful to be done by the AT handler for these
     request packets anymore at this point.
-- 
Stefan Richter
-=====-==--- --== =--==
http://arcgraph.de/sr/