[PATCH 0/3] firewire: fixes for kernel v4.9 or later
Hi,
This patchset respins patches posted before to fix some bugs for Linux FireWire subsystem. I expect them to be sent to Linus via pull request by maintainer of Linux sound subsystem since the path appears to be available after a short conversation with the maintainer. This patchset is expected to be applied to 'for-linus' branch for v5.18 kernel, and to stable kernels based on v4.9 or later.
This patchset includes below patches:
* [PATCH V2] drivers/firewire: use struct_size over open coded arithmetic * https://lore.kernel.org/lkml/20220210060805.1608198-1-chi.minghao@zte.com.cn... * [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset * https://lore.kernel.org/lkml/20220303183038.54126-1-dossche.niels@gmail.com/ * [PATCH] firewire: remove check of list iterator against head past the loop body * https://lore.kernel.org/lkml/20220331223601.902329-1-jakobkoschel@gmail.com/
Chengfeng Ye (1): firewire: fix potential uaf in outbound_phy_packet_callback()
Jakob Koschel (1): firewire: remove check of list iterator against head past the loop body
Niels Dossche (1): firewire: core: extend card->lock in fw_core_handle_bus_reset
drivers/firewire/core-card.c | 3 +++ drivers/firewire/core-cdev.c | 4 +++- drivers/firewire/core-topology.c | 9 +++------ drivers/firewire/core-transaction.c | 30 +++++++++++++++-------------- drivers/firewire/sbp2.c | 13 +++++++------ 5 files changed, 32 insertions(+), 27 deletions(-)
From: Chengfeng Ye cyeaa@connect.ust.hk
&e->event and e point to the same address, and &e->event could be freed in queue_event. So there is a potential uaf issue if we dereference e after calling queue_event(). Fix this by adding a temporary variable to maintain e->client in advance, this can avoid the potential uaf issue.
Cc: stable@vger.kernel.org Signed-off-by: Chengfeng Ye cyeaa@connect.ust.hk Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- drivers/firewire/core-cdev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 9f89c17730b1..708e417200f4 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1500,6 +1500,7 @@ static void outbound_phy_packet_callback(struct fw_packet *packet, { struct outbound_phy_packet_event *e = container_of(packet, struct outbound_phy_packet_event, p); + struct client *e_client;
switch (status) { /* expected: */ @@ -1516,9 +1517,10 @@ static void outbound_phy_packet_callback(struct fw_packet *packet, } e->phy_packet.data[0] = packet->timestamp;
+ e_client = e->client; queue_event(e->client, &e->event, &e->phy_packet, sizeof(e->phy_packet) + e->phy_packet.length, NULL, 0); - client_put(e->client); + client_put(e_client); }
static int ioctl_send_phy_packet(struct client *client, union ioctl_arg *arg)
From: Jakob Koschel jakobkoschel@gmail.com
When list_for_each_entry() completes the iteration over the whole list without breaking the loop, the iterator value will be a bogus pointer computed based on the head element.
While it is safe to use the pointer to determine if it was computed based on the head element, either with list_entry_is_head() or &pos->member == head, using the iterator variable after the loop should be avoided.
In preparation to limit the scope of a list iterator to the list traversal loop, use a dedicated pointer to point to the found element [1].
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWX... [1] Cc: stable@vger.kernel.org Signed-off-by: Jakob Koschel jakobkoschel@gmail.com Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- drivers/firewire/core-transaction.c | 30 +++++++++++++++-------------- drivers/firewire/sbp2.c | 13 +++++++------ 2 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/firewire/core-transaction.c b/drivers/firewire/core-transaction.c index ac487c96bb71..6c20815cc8d1 100644 --- a/drivers/firewire/core-transaction.c +++ b/drivers/firewire/core-transaction.c @@ -73,24 +73,25 @@ static int try_cancel_split_timeout(struct fw_transaction *t) static int close_transaction(struct fw_transaction *transaction, struct fw_card *card, int rcode) { - struct fw_transaction *t; + struct fw_transaction *t = NULL, *iter; unsigned long flags;
spin_lock_irqsave(&card->lock, flags); - list_for_each_entry(t, &card->transaction_list, link) { - if (t == transaction) { - if (!try_cancel_split_timeout(t)) { + list_for_each_entry(iter, &card->transaction_list, link) { + if (iter == transaction) { + if (!try_cancel_split_timeout(iter)) { spin_unlock_irqrestore(&card->lock, flags); goto timed_out; } - list_del_init(&t->link); - card->tlabel_mask &= ~(1ULL << t->tlabel); + list_del_init(&iter->link); + card->tlabel_mask &= ~(1ULL << iter->tlabel); + t = iter; break; } } spin_unlock_irqrestore(&card->lock, flags);
- if (&t->link != &card->transaction_list) { + if (t) { t->callback(card, rcode, NULL, 0, t->callback_data); return 0; } @@ -935,7 +936,7 @@ EXPORT_SYMBOL(fw_core_handle_request);
void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) { - struct fw_transaction *t; + struct fw_transaction *t = NULL, *iter; unsigned long flags; u32 *data; size_t data_length; @@ -947,20 +948,21 @@ void fw_core_handle_response(struct fw_card *card, struct fw_packet *p) rcode = HEADER_GET_RCODE(p->header[1]);
spin_lock_irqsave(&card->lock, flags); - list_for_each_entry(t, &card->transaction_list, link) { - if (t->node_id == source && t->tlabel == tlabel) { - if (!try_cancel_split_timeout(t)) { + list_for_each_entry(iter, &card->transaction_list, link) { + if (iter->node_id == source && iter->tlabel == tlabel) { + if (!try_cancel_split_timeout(iter)) { spin_unlock_irqrestore(&card->lock, flags); goto timed_out; } - list_del_init(&t->link); - card->tlabel_mask &= ~(1ULL << t->tlabel); + list_del_init(&iter->link); + card->tlabel_mask &= ~(1ULL << iter->tlabel); + t = iter; break; } } spin_unlock_irqrestore(&card->lock, flags);
- if (&t->link == &card->transaction_list) { + if (!t) { timed_out: fw_notice(card, "unsolicited response (source %x, tlabel %x)\n", source, tlabel); diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c index 85cd379fd383..60051c0cabea 100644 --- a/drivers/firewire/sbp2.c +++ b/drivers/firewire/sbp2.c @@ -408,7 +408,7 @@ static void sbp2_status_write(struct fw_card *card, struct fw_request *request, void *payload, size_t length, void *callback_data) { struct sbp2_logical_unit *lu = callback_data; - struct sbp2_orb *orb; + struct sbp2_orb *orb = NULL, *iter; struct sbp2_status status; unsigned long flags;
@@ -433,17 +433,18 @@ static void sbp2_status_write(struct fw_card *card, struct fw_request *request,
/* Lookup the orb corresponding to this status write. */ spin_lock_irqsave(&lu->tgt->lock, flags); - list_for_each_entry(orb, &lu->orb_list, link) { + list_for_each_entry(iter, &lu->orb_list, link) { if (STATUS_GET_ORB_HIGH(status) == 0 && - STATUS_GET_ORB_LOW(status) == orb->request_bus) { - orb->rcode = RCODE_COMPLETE; - list_del(&orb->link); + STATUS_GET_ORB_LOW(status) == iter->request_bus) { + iter->rcode = RCODE_COMPLETE; + list_del(&iter->link); + orb = iter; break; } } spin_unlock_irqrestore(&lu->tgt->lock, flags);
- if (&orb->link != &lu->orb_list) { + if (orb) { orb->callback(orb, &status); kref_put(&orb->kref, free_orb); /* orb callback reference */ } else {
From: Niels Dossche dossche.niels@gmail.com
card->local_node and card->bm_retries are both always accessed under card->lock. fw_core_handle_bus_reset has a check whose condition depends on card->local_node and whose body writes to card->bm_retries. Both of these accesses are not under card->lock. Move the lock acquiring of card->lock to before this check such that these accesses do happen when card->lock is held. fw_destroy_nodes is called inside the check. Since fw_destroy_nodes already acquires card->lock inside its function body, move this out to the callsites of fw_destroy_nodes. Also add a comment to indicate which locking is necessary when calling fw_destroy_nodes.
Cc: stable@vger.kernel.org Signed-off-by: Niels Dossche dossche.niels@gmail.com Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- drivers/firewire/core-card.c | 3 +++ drivers/firewire/core-topology.c | 9 +++------ 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c index 54be88167c60..f3b3953cac83 100644 --- a/drivers/firewire/core-card.c +++ b/drivers/firewire/core-card.c @@ -668,6 +668,7 @@ EXPORT_SYMBOL_GPL(fw_card_release); void fw_core_remove_card(struct fw_card *card) { struct fw_card_driver dummy_driver = dummy_driver_template; + unsigned long flags;
card->driver->update_phy_reg(card, 4, PHY_LINK_ACTIVE | PHY_CONTENDER, 0); @@ -682,7 +683,9 @@ void fw_core_remove_card(struct fw_card *card) dummy_driver.stop_iso = card->driver->stop_iso; card->driver = &dummy_driver;
+ spin_lock_irqsave(&card->lock, flags); fw_destroy_nodes(card); + spin_unlock_irqrestore(&card->lock, flags);
/* Wait for all users, especially device workqueue jobs, to finish. */ fw_card_put(card); diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c index b63d55f5ebd3..f40c81534381 100644 --- a/drivers/firewire/core-topology.c +++ b/drivers/firewire/core-topology.c @@ -375,16 +375,13 @@ static void report_found_node(struct fw_card *card, card->bm_retries = 0; }
+/* Must be called with card->lock held */ void fw_destroy_nodes(struct fw_card *card) { - unsigned long flags; - - spin_lock_irqsave(&card->lock, flags); card->color++; if (card->local_node != NULL) for_each_fw_node(card, card->local_node, report_lost_node); card->local_node = NULL; - spin_unlock_irqrestore(&card->lock, flags); }
static void move_tree(struct fw_node *node0, struct fw_node *node1, int port) @@ -510,6 +507,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, struct fw_node *local_node; unsigned long flags;
+ spin_lock_irqsave(&card->lock, flags); + /* * If the selfID buffer is not the immediate successor of the * previously processed one, we cannot reliably compare the @@ -521,8 +520,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation, card->bm_retries = 0; }
- spin_lock_irqsave(&card->lock, flags); - card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated; card->node_id = node_id; /*
On Sat, 09 Apr 2022 06:12:40 +0200, Takashi Sakamoto wrote:
Hi,
This patchset respins patches posted before to fix some bugs for Linux FireWire subsystem. I expect them to be sent to Linus via pull request by maintainer of Linux sound subsystem since the path appears to be available after a short conversation with the maintainer. This patchset is expected to be applied to 'for-linus' branch for v5.18 kernel, and to stable kernels based on v4.9 or later.
This patchset includes below patches:
- [PATCH V2] drivers/firewire: use struct_size over open coded arithmetic
- [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset
- [PATCH] firewire: remove check of list iterator against head past the loop body
Chengfeng Ye (1): firewire: fix potential uaf in outbound_phy_packet_callback()
Jakob Koschel (1): firewire: remove check of list iterator against head past the loop body
Niels Dossche (1): firewire: core: extend card->lock in fw_core_handle_bus_reset
Now I applied all those pending patches to topic/firewire branch, merged into for-linus branch.
thanks,
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto