[PATCH v2 0/2] platform/surface: aggregator: Fix packet list handling in ssh_ptl_shutdown()

[PATCH v2 0/2] platform/surface: aggregator: Fix packet list handling in ssh_ptl_shutdown()

[Wentao Liang]

Hi Maximilian, Ilpo,

this is a split of the single patch I sent in September, following the
review comments from Ilpo and Maximilian. The two problems in
ssh_ptl_shutdown() are independent, so patch 1 corrects the list that
pending packets are moved to, and patch 2 makes both drain loops safe
against the packet being released while iterating.

Changes in v2:
 - Sorry for the delay in getting back to this review.
 - Split into two patches as requested, instead of one patch that mixes
   two independent problems.
 - The pending/complete list mismatch has its own changelog describing
   the effect of the wrong list node, as Maximilian asked for.
 - Both patches carry Maximilian's Acked-by.

Link: https://lore.kernel.org/all/20260917135607.2154130-1-vulab@iscas.ac.cn/

The candidate was found with our static analysis tooling; the audit and
the fix were then assisted by DeepSeek and GPT-6.1 Sol. The patches are
based on mainline as of d4872371bae (v7.4-rc1) and were built for x86_64
with CONFIG_SURFACE_AGGREGATOR=m, without new warnings; checkpatch.pl
reports no issues. I could not test them at runtime, as that needs a
Surface device shutting down with packets still queued or pending.

Wentao Liang (2):
  platform/surface: aggregator: Fix wrong completion list for pending
    packets
  platform/surface: aggregator: Fix use-after-free in ssh_ptl_shutdown()

 drivers/platform/surface/aggregator/ssh_packet_layer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v2 2/2] platform/surface: aggregator: Fix use-after-free in ssh_ptl_shutdown()

[Wentao Liang]

Both loops that drain the local packet lists in ssh_ptl_shutdown() call
ssh_packet_put() on the loop variable. This drops the reference that the
queue or the pending set held on the packet, which can be the last one,
so the packet may be freed before the iterator advances to the next
entry. The iterator then reads the list node of the freed packet to do
so.

Use list_for_each_entry_safe() for both loops, which caches the next
entry while the current packet is still alive.

Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Cc: stable@vger.kernel.org
Acked-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/platform/surface/aggregator/ssh_packet_layer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index a51395942747..871780828419 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -1989,7 +1989,7 @@ void ssh_ptl_shutdown(struct ssh_ptl *ptl)
 	spin_unlock(&ptl->pending.lock);
 
 	/* Complete and drop packets on complete_q. */
-	list_for_each_entry(p, &complete_q, queue_node) {
+	list_for_each_entry_safe(p, n, &complete_q, queue_node) {
 		if (!test_and_set_bit(SSH_PACKET_SF_COMPLETED_BIT, &p->state))
 			__ssh_ptl_complete(p, -ESHUTDOWN);
 
@@ -1997,7 +1997,7 @@ void ssh_ptl_shutdown(struct ssh_ptl *ptl)
 	}
 
 	/* Complete and drop packets on complete_p. */
-	list_for_each_entry(p, &complete_p, pending_node) {
+	list_for_each_entry_safe(p, n, &complete_p, pending_node) {
 		if (!test_and_set_bit(SSH_PACKET_SF_COMPLETED_BIT, &p->state))
 			__ssh_ptl_complete(p, -ESHUTDOWN);
 

-- 
2.34.1

[PATCH v2 1/2] platform/surface: aggregator: Fix wrong completion list for pending packets

[Wentao Liang]

ssh_ptl_shutdown() takes the packets it has to cancel out of the queue
and out of the pending set and collects them in two local lists, which
are drained once both locks have been dropped. Packets from the pending
set are moved to complete_p through pending_node, as their queue_node
may still be in use by the queue when a pending packet is re-queued for
retransmission. The move itself, however, targets complete_q.

An entry added that way is linked into complete_q via pending_node,
while the loop that drains complete_q looks the entry up with
queue_node. The two nodes are at different offsets in struct
ssh_packet, so list_for_each_entry() derives a packet pointer that is
shifted by that difference: the state bit is updated at the wrong
offset and the completion and reference-drop run on an object that is
not the packet the list entry belongs to. At the same time complete_p
stays empty and its drain loop never runs.

Move the pending packets to complete_p as intended.

Fixes: c167b9c7e3d6 ("platform/surface: Add Surface Aggregator subsystem")
Cc: stable@vger.kernel.org
Acked-by: Maximilian Luz <luzmaximilian@gmail.com>
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/platform/surface/aggregator/ssh_packet_layer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/surface/aggregator/ssh_packet_layer.c b/drivers/platform/surface/aggregator/ssh_packet_layer.c
index 3dd22856570f..a51395942747 100644
--- a/drivers/platform/surface/aggregator/ssh_packet_layer.c
+++ b/drivers/platform/surface/aggregator/ssh_packet_layer.c
@@ -1983,7 +1983,7 @@ void ssh_ptl_shutdown(struct ssh_ptl *ptl)
 		smp_mb__before_atomic();
 		clear_bit(SSH_PACKET_SF_PENDING_BIT, &p->state);
 
-		list_move_tail(&p->pending_node, &complete_q);
+		list_move_tail(&p->pending_node, &complete_p);
 	}
 	atomic_set(&ptl->pending.count, 0);
 	spin_unlock(&ptl->pending.lock);

-- 
2.34.1