1. Patchew
  2. linux
  3. View series

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

Alexey Simakov posted 1 patch 2 months ago
There is a newer version of this series
net/sctp/inqueue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
Posted by Alexey Simakov 2 months ago 
chunk->skb pointer is dereferenced in the if-block where it's supposed
to be NULL only.

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)
 				chunk->skb = skb_shinfo(chunk->skb)->frag_list;
 
 			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
Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
Posted by Marcelo Ricardo Leitner 2 months ago 
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;
>  			}
> -- 
> 2.34.1
>
Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
Posted by Alexey Simakov 2 months ago 
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.
				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;
			}
		}
Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
Posted by Marcelo Ricardo Leitner 2 months ago 
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;
> 			}
> 		}

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.