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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Fri, Oct 17, 2025 at 10:15:50AM +0300, Alexey Simakov wrote:
> 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:
> > > chunk->skb pointer is dereferenced in the if-block where it's supposed
> > > to be NULL only.
> > 
> > The issue is well spotted. More below.
> > 
> > > 
> > > 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 <bigalex934@gmail.com>
> > > ---
> > >  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;
> > >  			}
> I'm not sure, that correctly understand the new location of updated check.
> There a few assumtions below.
> > > -- 
> > > 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.

Exactly.

> 				chunk->skb = skb_shinfo(chunk->skb)->frag_list;
> Adding here could make sense, chunk->skb changed => do something if it became null.

Yes. But then it needs to restore chunk->skb somehow. So instead it's better
to do the WARN_ON(!skb_shinfo(chunk->skb)->frag_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) {
		if (WARN_ON(!skb_shinfo(chunk->skb)->frag_list)) {
			__SCTP_INC_STATS(dev_net(chunk->skb->dev),
			                                 ^-- can be skb again
					 SCTP_MIB_IN_PKT_DISCARDS);
			sctp_chunk_free(chunk);
			   ^---- so this can actually free chunk->skb
			goto next_chunk;
		}
		chunk->skb = skb_shinfo(chunk->skb)->frag_list;
	}
}

Makes sense?

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