[PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[Xin Long]

A null pointer reference issue can be triggered when the response of a
stream reconf request arrives after the timer is triggered, such as:

  send Incoming SSN Reset Request --->
  CPU0:
   reconf timer is triggered,
   go to the handler code before hold sk lock
                            <--- reply with Outgoing SSN Reset Request
  CPU1:
   process Outgoing SSN Reset Request,
   and set asoc->strreset_chunk to NULL
  CPU0:
   continue the handler code, hold sk lock,
   and try to hold asoc->strreset_chunk, crash!

In Ying Xu's testing, the call trace is:

  [ ] BUG: kernel NULL pointer dereference, address: 0000000000000010
  [ ] RIP: 0010:sctp_chunk_hold+0xe/0x40 [sctp]
  [ ] Call Trace:
  [ ]  <IRQ>
  [ ]  sctp_sf_send_reconf+0x2c/0x100 [sctp]
  [ ]  sctp_do_sm+0xa4/0x220 [sctp]
  [ ]  sctp_generate_reconf_event+0xbd/0xe0 [sctp]
  [ ]  call_timer_fn+0x26/0x130

This patch is to fix it by returning from the timer handler if asoc
strreset_chunk is already set to NULL.

Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
Reported-by: Ying Xu <yinxu@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_sideeffect.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index b3815b568e8e..463c4a58d2c3 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -458,6 +458,10 @@ void sctp_generate_reconf_event(struct timer_list *t)
 		goto out_unlock;
 	}
 
+	/* This happens when the response arrives after the timer is triggered. */
+	if (!asoc->strreset_chunk)
+		goto out_unlock;
+
 	error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
 			   SCTP_ST_TIMEOUT(SCTP_EVENT_TIMEOUT_RECONF),
 			   asoc->state, asoc->ep, asoc,
-- 
2.31.1

Re: [PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 20 Apr 2022 16:52:41 -0400

> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index b3815b568e8e..463c4a58d2c3 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -458,6 +458,10 @@ void sctp_generate_reconf_event(struct timer_list *t)
>  		goto out_unlock;
>  	}
>  
> +	/* This happens when the response arrives after the timer is triggered. */
> +	if (!asoc->strreset_chunk)
> +		goto out_unlock;
> +
This will return 0 because that is error's value right there, intentional?

Thanks.

Re: [PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[Xin Long]

On Thu, Apr 21, 2022 at 6:18 AM David Miller <davem@davemloft.net> wrote:
>
> From: Xin Long <lucien.xin@gmail.com>
> Date: Wed, 20 Apr 2022 16:52:41 -0400
>
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index b3815b568e8e..463c4a58d2c3 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -458,6 +458,10 @@ void sctp_generate_reconf_event(struct timer_list *t)
> >               goto out_unlock;
> >       }
> >
> > +     /* This happens when the response arrives after the timer is triggered. */
> > +     if (!asoc->strreset_chunk)
> > +             goto out_unlock;
> > +
> This will return 0 because that is error's value right there, intentional?
intentional, this won't be treated as an error.

Re: [PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[Marcelo Ricardo Leitner]

On Wed, Apr 20, 2022 at 04:52:41PM -0400, Xin Long wrote:
> A null pointer reference issue can be triggered when the response of a
> stream reconf request arrives after the timer is triggered, such as:
> 
>   send Incoming SSN Reset Request --->
>   CPU0:
>    reconf timer is triggered,
>    go to the handler code before hold sk lock
>                             <--- reply with Outgoing SSN Reset Request
>   CPU1:
>    process Outgoing SSN Reset Request,
>    and set asoc->strreset_chunk to NULL
>   CPU0:
>    continue the handler code, hold sk lock,
>    and try to hold asoc->strreset_chunk, crash!
> 
> In Ying Xu's testing, the call trace is:
> 
>   [ ] BUG: kernel NULL pointer dereference, address: 0000000000000010
>   [ ] RIP: 0010:sctp_chunk_hold+0xe/0x40 [sctp]
>   [ ] Call Trace:
>   [ ]  <IRQ>
>   [ ]  sctp_sf_send_reconf+0x2c/0x100 [sctp]
>   [ ]  sctp_do_sm+0xa4/0x220 [sctp]
>   [ ]  sctp_generate_reconf_event+0xbd/0xe0 [sctp]
>   [ ]  call_timer_fn+0x26/0x130
> 
> This patch is to fix it by returning from the timer handler if asoc
> strreset_chunk is already set to NULL.

Right. The timer callback didn't have a check on whether it was still
needed or not, and per the description above, it would simply try to
handle it twice then.

> 
> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> Reported-by: Ying Xu <yinxu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 20 Apr 2022 16:52:41 -0400 you wrote:
> A null pointer reference issue can be triggered when the response of a
> stream reconf request arrives after the timer is triggered, such as:
> 
>   send Incoming SSN Reset Request --->
>   CPU0:
>    reconf timer is triggered,
>    go to the handler code before hold sk lock
>                             <--- reply with Outgoing SSN Reset Request
>   CPU1:
>    process Outgoing SSN Reset Request,
>    and set asoc->strreset_chunk to NULL
>   CPU0:
>    continue the handler code, hold sk lock,
>    and try to hold asoc->strreset_chunk, crash!
> 
> [...]

Here is the summary with links:
  - [net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event
    https://git.kernel.org/netdev/net/c/165e3e17fe8f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html