[PATCH net v2] sctp: fix stream->outcnt underflow on duplicate RECONF responses

From: Jun Yang <[email protected]>

sctp_process_strreset_resp() rolls back a denied ADD_OUT_STREAMS request
by subtracting the requested count from the current stream count:

  nums   = ntohs(addstrm->number_of_streams);
  number = stream->outcnt - nums;          /* net/sctp/stream.c:1050 */
  ...
  stream->outcnt = number;                 /* net/sctp/stream.c:1060 */

This undoes the increment sctp_send_add_streams() performed at request
time, and is only correct if it runs exactly once per request.  Nothing
enforces that.

asoc->strreset_outstanding is a plain count of the request parameters on
the fly, so it cannot tell which request a response belongs to, and
sctp_chunk_lookup_strreset_param() accepts any parameter still present
in asoc->strreset_chunk, which stays live until that count reaches zero.
When both outgoing and incoming streams are added in one
setsockopt(SCTP_ADD_STREAMS), sctp_send_add_streams() sets the count to
2 and caches a chunk holding both the ADD_OUT and ADD_IN parameters.

sctp_verify_reconf() permits a RESET_RESPONSE to follow another
RESET_RESPONSE, so a peer can put two responses carrying the ADD_OUT
request_seq into a single RECONF chunk.  sctp_sf_do_reconf() processes
both: the first rollback restores the original count and the second
subtracts nums again.  Depending on the counts, this either wraps the
__u16 or silently shrinks the stream count a second time.

SCTP_SO() is genradix_ptr(), which returns NULL past the preallocated
range, so sctp_sendmsg_to_asoc() accepts an out-of-range stream id at
net/sctp/socket.c:1803 and dereferences the resulting NULL slot at
net/sctp/socket.c:1808.  Other stream walkers likewise trust the
inflated count until a later operation repairs or tears down the
association.

Turn strreset_outstanding into a bitmask of the request parameter types
on the fly, one bit per SCTP_PARAM_RESET_* request type, and process a
response only if its request type is still outstanding.  Handling a
response clears its bit, so a duplicate is dropped, while responses for
the other parameters of the same RECONF chunk are still accepted in any
order.

Test the bit in sctp_process_strreset_outreq() and
sctp_process_strreset_addstrm_out() as well: a peer request that
implicitly answers our IN/ADD_IN request could otherwise answer it
twice, retiring the cached chunk and its reconf timer while another
request was still waiting for a response.

Fixes: 11ae76e67a17 ("sctp: implement receiver-side procedures for the Reconf Response Parameter")
Cc: [email protected]
Reported-by: TencentOS Corvus AI <[email protected]>
Suggested-by: Xin Long <[email protected]>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <[email protected]>
---
A KASAN reproducer for this issue is available if requested.

v2:
 - Drop the "resp->response_seq != htonl(asoc->strreset_outseq)" test: an
   ADD_IN/IN request uses outseq + 1, so it rejected a valid response
   that arrives before the ADD_OUT/OUT one.  Drop the
   !strreset_outstanding test as well, it does not stop a duplicate
   while another parameter of the same chunk is still on the fly
   (Xin Long).
 - Instead track the requests on the fly as a per request type bitmask
   and test it in sctp_process_strreset_resp(), so a duplicate is
   rejected per request type, regardless of the arrival order.
 - Drop the "nums > stream->outcnt" test, no longer needed once the
   duplicate response is rejected (Xin Long).

v1: https://lore.kernel.org/netdev/[email protected]/

 include/net/sctp/structs.h | 16 +++++++++++++++-
 net/sctp/stream.c          | 30 +++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index cccc662561aa..0b48c45d647e 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -2057,7 +2057,7 @@ struct sctp_association {
 	     force_delay:1;
 
 	__u8 strreset_enable;
-	__u8 strreset_outstanding; /* request param count on the fly */
+	__u8 strreset_outstanding; /* request param bitmask on the fly */
 
 	__u32 strreset_outseq; /* Update after receiving response */
 	__u32 strreset_inseq; /* Update after receiving request */
@@ -2087,6 +2087,20 @@ struct sctp_association {
 	struct rcu_head rcu;
 };
 
+/* Track the outstanding stream reconf requests per request param type, so
+ * that a response can only clear the request it belongs to, and only once.
+ * The reconf param types range from SCTP_PARAM_RESET_OUT_REQUEST (0x000d)
+ * to SCTP_PARAM_RESET_ADD_IN_STREAMS (0x0012), so one bit each fits in
+ * asoc->strreset_outstanding.
+ */
+#define SCTP_STRRESET_MASK(type) \
+	(1 << (ntohs(type) - ntohs(SCTP_PARAM_RESET_OUT_REQUEST)))
+#define SCTP_STRRESET_TEST(asoc, type) \
+	((asoc)->strreset_outstanding & SCTP_STRRESET_MASK(type))
+#define SCTP_STRRESET_SET(asoc, type) \
+	((asoc)->strreset_outstanding |= SCTP_STRRESET_MASK(type))
+#define SCTP_STRRESET_CLEAR(asoc, type) \
+	((asoc)->strreset_outstanding &= ~SCTP_STRRESET_MASK(type))
 
 /* An eyecatcher for determining if we are really looking at an
  * association data structure.
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 34ffe6c945a4..cb7543929d65 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -372,7 +372,10 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 		goto out;
 	}
 
-	asoc->strreset_outstanding = out + in;
+	if (out)
+		SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_OUT_REQUEST);
+	if (in)
+		SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_IN_REQUEST);
 
 out:
 	return retval;
@@ -417,7 +420,7 @@ int sctp_send_reset_assoc(struct sctp_association *asoc)
 		return retval;
 	}
 
-	asoc->strreset_outstanding = 1;
+	SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_TSN_REQUEST);
 
 	return 0;
 }
@@ -474,7 +477,10 @@ int sctp_send_add_streams(struct sctp_association *asoc,
 		goto out;
 	}
 
-	asoc->strreset_outstanding = !!out + !!in;
+	if (out)
+		SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_ADD_OUT_STREAMS);
+	if (in)
+		SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_ADD_IN_STREAMS);
 
 out:
 	return retval;
@@ -564,13 +570,14 @@ struct sctp_chunk *sctp_process_strreset_outreq(
 	if (asoc->strreset_chunk) {
 		if (!sctp_chunk_lookup_strreset_param(
 				asoc, outreq->response_seq,
-				SCTP_PARAM_RESET_IN_REQUEST)) {
+				SCTP_PARAM_RESET_IN_REQUEST) ||
+		    !SCTP_STRRESET_TEST(asoc, SCTP_PARAM_RESET_IN_REQUEST)) {
 			/* same process with outstanding isn't 0 */
 			result = SCTP_STRRESET_ERR_IN_PROGRESS;
 			goto out;
 		}
 
-		asoc->strreset_outstanding--;
+		SCTP_STRRESET_CLEAR(asoc, SCTP_PARAM_RESET_IN_REQUEST);
 		asoc->strreset_outseq++;
 
 		if (!asoc->strreset_outstanding) {
@@ -669,7 +676,7 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 			SCTP_SO(stream, i)->state = SCTP_STREAM_CLOSED;
 
 	asoc->strreset_chunk = chunk;
-	asoc->strreset_outstanding = 1;
+	SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_OUT_REQUEST);
 	sctp_chunk_hold(asoc->strreset_chunk);
 
 	result = SCTP_STRRESET_PERFORMED;
@@ -816,13 +823,14 @@ struct sctp_chunk *sctp_process_strreset_addstrm_out(
 
 	if (asoc->strreset_chunk) {
 		if (!sctp_chunk_lookup_strreset_param(
-			asoc, 0, SCTP_PARAM_RESET_ADD_IN_STREAMS)) {
+			asoc, 0, SCTP_PARAM_RESET_ADD_IN_STREAMS) ||
+		    !SCTP_STRRESET_TEST(asoc, SCTP_PARAM_RESET_ADD_IN_STREAMS)) {
 			/* same process with outstanding isn't 0 */
 			result = SCTP_STRRESET_ERR_IN_PROGRESS;
 			goto out;
 		}
 
-		asoc->strreset_outstanding--;
+		SCTP_STRRESET_CLEAR(asoc, SCTP_PARAM_RESET_ADD_IN_STREAMS);
 		asoc->strreset_outseq++;
 
 		if (!asoc->strreset_outstanding) {
@@ -899,7 +907,7 @@ struct sctp_chunk *sctp_process_strreset_addstrm_in(
 		goto out;
 
 	asoc->strreset_chunk = chunk;
-	asoc->strreset_outstanding = 1;
+	SCTP_STRRESET_SET(asoc, SCTP_PARAM_RESET_ADD_OUT_STREAMS);
 	sctp_chunk_hold(asoc->strreset_chunk);
 
 	stream->outcnt = outcnt;
@@ -928,7 +936,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
 	__u32 result;
 
 	req = sctp_chunk_lookup_strreset_param(asoc, resp->response_seq, 0);
-	if (!req)
+	if (!req || !SCTP_STRRESET_TEST(asoc, req->type))
 		return NULL;
 
 	result = ntohl(resp->result);
@@ -1078,7 +1086,7 @@ struct sctp_chunk *sctp_process_strreset_resp(
 			nums, 0, GFP_ATOMIC);
 	}
 
-	asoc->strreset_outstanding--;
+	SCTP_STRRESET_CLEAR(asoc, req->type);
 	asoc->strreset_outseq++;
 
 	/* remove everything for this reconf request */
-- 
2.55.0