On Wed, Oct 15, 2025 at 04:50:07PM -0300, Marcelo Ricardo Leitner wrote:

On Wed, Oct 15, 2025 at 09:45:10PM +0300, Alexey Simakov wrote:

quoted

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

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.

quoted

 				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

quoted

 
 			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;
 			}

I'm not sure, that correctly understand the new location of updated check.
There a few assumtions below.

quoted

-- 
2.34.1

		/* Is the queue empty?  */
		entry = sctp_list_dequeue(&queue->in_chunk_list);
		if (!entry)
			return NULL;

		chunk = list_entry(entry, struct sctp_chunk, list);

		if (skb_is_gso(chunk->skb) && skb_is_gso_sctp(chunk->skb)) {
			/* 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)
Adding this check here will not fix problem, since chunk->skb always true here because it dereferencing in
checks above.
				chunk->skb = skb_shinfo(chunk->skb)->frag_list;
Adding here could make sense, chunk->skb changed => do something if it became null.

			if (WARN_ON(!chunk->skb)) {
				__SCTP_INC_STATS(dev_net(chunk->head_skb->dev),
						 SCTP_MIB_IN_PKT_DISCARDS);
				sctp_chunk_free(chunk);
				goto next_chunk;
			}
		}