Thread (4 messages) 4 messages, 2 authors, 2025-10-17

Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2025-10-15 19:50:11
Also in: linux-sctp, lkml

On Wed, Oct 15, 2025 at 09:45:10PM +0300, Alexey Simakov wrote:
chunk->skb pointer is dereferenced in the if-block where it's supposed
to be NULL only.
The issue is well spotted. More below.
quoted hunk ↗ jump to hunk
Use the chunk header instead, which should be available at this point
in execution.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 90017accff61 ("sctp: Add GSO support")
Signed-off-by: Alexey Simakov <redacted>
---
 net/sctp/inqueue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
index 5c1652181805..f1830c21953f 100644
--- a/net/sctp/inqueue.c
+++ b/net/sctp/inqueue.c
@@ -173,7 +173,8 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
With more context here:

               if ((skb_shinfo(chunk->skb)->gso_type & SKB_GSO_SCTP) == SKB_GSO_SCTP) {
                       /* GSO-marked skbs but without frags, handle
                        * them normally
                        */

                       if (skb_shinfo(chunk->skb)->frag_list)
                               chunk->head_skb = chunk->skb;

                       /* skbs with "cover letter" */
                       if (chunk->head_skb && chunk->skb->data_len == chunk->skb->len)
		           ^^^^^^^^^^^^^^^^^^

chunk->head_skb would also not be guaranteed.
 				chunk->skb = skb_shinfo(chunk->skb)->frag_list;
But chunk->skb can only be NULL if chunk->head_skb is not, then.

Thing is, we cannot replace chunk->skb here then, because otherwise
when freeing this chunk in sctp_chunk_free below it will not reference
chunk->head_skb and will cause a leak.

With that, the check below should be done just before replacing
chunk->skb right above, inside the if() block. We're sure that
otherwise chunk->skb is non-NULL because of outer if() condition.

Thanks,
Marcelo
 
 			if (WARN_ON(!chunk->skb)) {
-				__SCTP_INC_STATS(dev_net(chunk->skb->dev), SCTP_MIB_IN_PKT_DISCARDS);
+				__SCTP_INC_STATS(dev_net(chunk->head_skb->dev),
+						 SCTP_MIB_IN_PKT_DISCARDS);
 				sctp_chunk_free(chunk);
 				goto next_chunk;
 			}
-- 
2.34.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help