by Stefan Richter

[permalink] [raw]

Subject: Re: [PATCH 1/5] firewire: fix use of multiple AV/C devices, allow multiple FCP listeners

2010-01-24 15:45:33

by Stefan Richter

[permalink] [raw]

Subject: [PATCH] firewire: core: fix use-after-free regression in FCP handler

Commit db5d247a "firewire: fix use of multiple AV/C devices, allow
multiple FCP listeners" introduced a regression into 2.6.33-rc3:
The core freed payloads of incoming requests to FCP_Request or
FCP_Response before a userspace driver accessed them.

We need to copy such payloads for each registered userspace client
and free the copies according to the lifetime rules of non-FCP client
request resources.

(This could possibly be optimized by reference counts instead of
copies.)

The presently only kernelspace driver which listens for FCP requests,
firedtv, was not affected because it already copies FCP frames into an
own buffer before returning to firewire-core's FCP handler dispatcher.

Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/core-cdev.c | 50 +++++++++++++++++++++++++----------
1 file changed, 36 insertions(+), 14 deletions(-)

Index: linux-2.6.32.2/drivers/firewire/core-cdev.c
===================================================================
--- linux-2.6.32.2.orig/drivers/firewire/core-cdev.c
+++ linux-2.6.32.2/drivers/firewire/core-cdev.c
@@ -35,6 +35,7 @@
#include <linux/preempt.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
+#include <linux/string.h>
#include <linux/time.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
@@ -595,13 +596,20 @@ static int ioctl_send_request(struct cli
client->device->max_speed);
}

+static inline bool is_fcp_request(struct fw_request *request)
+{
+ return request == NULL;
+}
+
static void release_request(struct client *client,
struct client_resource *resource)
{
struct inbound_transaction_resource *r = container_of(resource,
struct inbound_transaction_resource, resource);

- if (r->request)
+ if (is_fcp_request(r->request))
+ kfree(r->data);
+ else
fw_send_response(client->device->card, r->request,
RCODE_CONFLICT_ERROR);
kfree(r);
@@ -616,6 +624,7 @@ static void handle_request(struct fw_car
struct address_handler_resource *handler = callback_data;
struct inbound_transaction_resource *r;
struct inbound_transaction_event *e;
+ void *fcp_frame = NULL;
int ret;

r = kmalloc(sizeof(*r), GFP_ATOMIC);
@@ -627,6 +636,18 @@ static void handle_request(struct fw_car
r->data = payload;
r->length = length;

+ if (is_fcp_request(request)) {
+ /*
+ * FIXME: Let core-transaction.c manage a
+ * single reference-counted copy?
+ */
+ fcp_frame = kmemdup(payload, length, GFP_ATOMIC);
+ if (fcp_frame == NULL)
+ goto failed;
+
+ r->data = fcp_frame;
+ }
+
r->resource.release = release_request;
ret = add_client_resource(handler->client, &r->resource, GFP_ATOMIC);
if (ret < 0)
@@ -640,13 +661,15 @@ static void handle_request(struct fw_car
e->request.closure = handler->closure;

queue_event(handler->client, &e->event,
- &e->request, sizeof(e->request), payload, length);
+ &e->request, sizeof(e->request), r->data, length);
return;

failed:
kfree(r);
kfree(e);
- if (request)
+ kfree(fcp_frame);
+
+ if (!is_fcp_request(request))
fw_send_response(card, request, RCODE_CONFLICT_ERROR);
}

@@ -717,18 +740,17 @@ static int ioctl_send_response(struct cl

r = container_of(resource, struct inbound_transaction_resource,
resource);
- if (r->request) {
- if (request->length < r->length)
- r->length = request->length;
- if (copy_from_user(r->data, u64_to_uptr(request->data),
- r->length)) {
- ret = -EFAULT;
- kfree(r->request);
- goto out;
- }
- fw_send_response(client->device->card, r->request,
- request->rcode);
+ if (is_fcp_request(r->request))
+ goto out;
+
+ if (request->length < r->length)
+ r->length = request->length;
+ if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) {
+ ret = -EFAULT;
+ kfree(r->request);
+ goto out;
}
+ fw_send_response(client->device->card, r->request, request->rcode);
out:
kfree(r);

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