* [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
@ 2025-10-15 18:45 Alexey Simakov
2025-10-15 19:50 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Simakov @ 2025-10-15 18:45 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Alexey Simakov, Xin Long, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, linux-sctp, netdev,
linux-kernel, lvc-project
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
2025-10-15 18:45 [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing Alexey Simakov
@ 2025-10-15 19:50 ` Marcelo Ricardo Leitner
2025-10-17 7:15 ` Alexey Simakov
0 siblings, 1 reply; 4+ messages in thread
From: Marcelo Ricardo Leitner @ 2025-10-15 19:50 UTC (permalink / raw)
To: Alexey Simakov
Cc: Xin Long, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-sctp, netdev, linux-kernel,
lvc-project
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
2025-10-15 19:50 ` Marcelo Ricardo Leitner
@ 2025-10-17 7:15 ` Alexey Simakov
2025-10-17 11:06 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 4+ messages in thread
From: Alexey Simakov @ 2025-10-17 7:15 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Xin Long, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-sctp, netdev, linux-kernel,
lvc-project
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;
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing
2025-10-17 7:15 ` Alexey Simakov
@ 2025-10-17 11:06 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 4+ messages in thread
From: Marcelo Ricardo Leitner @ 2025-10-17 11:06 UTC (permalink / raw)
To: Alexey Simakov
Cc: Xin Long, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-sctp, netdev, linux-kernel,
lvc-project
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;
> }
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-17 11:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-15 18:45 [PATCH net] sctp: avoid NULL dereference when chunk data buffer is missing Alexey Simakov
2025-10-15 19:50 ` Marcelo Ricardo Leitner
2025-10-17 7:15 ` Alexey Simakov
2025-10-17 11:06 ` Marcelo Ricardo Leitner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).