From db5d247ae811f49185a71e703b65acad845e4b18 Mon Sep 17 00:00:00 2001 From: Clemens Ladisch Date: Thu, 24 Dec 2009 12:05:58 +0100 Subject: firewire: fix use of multiple AV/C devices, allow multiple FCP listeners Control of more than one AV/C device at once --- e.g. camcorders, tape decks, audio devices, TV tuners --- failed or worked only unreliably, depending on driver implementation. This affected kernelspace and userspace drivers alike and was caused by firewire-core's inability to accept multiple registrations of FCP listeners. The fix allows multiple address handlers to be registered for the FCP command and response registers. When a request for these registers is received, all handlers are invoked, and the Firewire response is generated by the core and not by any handler. The cdev API does not change, i.e., userspace is still expected to send a response for FCP requests; this response is silently ignored. Signed-off-by: Clemens Ladisch Signed-off-by: Stefan Richter (changelog, rebased, whitespace) --- drivers/firewire/core-cdev.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 231e6ee5ba43..2cb22d160f6e 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -601,8 +601,9 @@ static void release_request(struct client *client, struct inbound_transaction_resource *r = container_of(resource, struct inbound_transaction_resource, resource); - fw_send_response(client->device->card, r->request, - RCODE_CONFLICT_ERROR); + if (r->request) + fw_send_response(client->device->card, r->request, + RCODE_CONFLICT_ERROR); kfree(r); } @@ -645,7 +646,8 @@ static void handle_request(struct fw_card *card, struct fw_request *request, failed: kfree(r); kfree(e); - fw_send_response(card, request, RCODE_CONFLICT_ERROR); + if (request) + fw_send_response(card, request, RCODE_CONFLICT_ERROR); } static void release_address_handler(struct client *client, @@ -715,15 +717,17 @@ static int ioctl_send_response(struct client *client, void *buffer) r = container_of(resource, struct inbound_transaction_resource, resource); - if (request->length < r->length) - r->length = request->length; - - if (copy_from_user(r->data, u64_to_uptr(request->data), r->length)) { - ret = -EFAULT; - goto out; + 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; + goto out; + } + fw_send_response(client->device->card, r->request, + request->rcode); } - - fw_send_response(client->device->card, r->request, request->rcode); out: kfree(r); -- cgit v1.2.3 From cf0e575dcc4cab9fd955e9bec49df7e8ee30a7cf Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 26 Dec 2009 01:34:29 +0100 Subject: firewire: cdev: fix another memory leak in an error path If copy_from_user in an FW_CDEV_IOC_SEND_RESPONSE ioctl failed, the fw_request pointed to by the inbound_transaction_resource is no longer referenced and needs to be freed. Signed-off-by: Stefan Richter --- drivers/firewire/core-cdev.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 2cb22d160f6e..e6d63849e78e 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -723,6 +723,7 @@ static int ioctl_send_response(struct client *client, void *buffer) 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, -- cgit v1.2.3 From 281e20323ab72180137824a298ee9e21e6f9acf6 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sun, 24 Jan 2010 16:45:03 +0100 Subject: 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 --- drivers/firewire/core-cdev.c | 50 +++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 14 deletions(-) (limited to 'drivers/firewire/core-cdev.c') diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index e6d63849e78e..4eeaed57e219 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -595,13 +596,20 @@ static int ioctl_send_request(struct client *client, void *buffer) 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_card *card, struct fw_request *request, 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_card *card, struct fw_request *request, 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_card *card, struct fw_request *request, 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 client *client, void *buffer) 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); -- cgit v1.2.3