| Message ID | 3000f8b12920ae81b84dceead6dcc90bb00c0403.1650487961.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 165e3e17fe8fe6a8aab319bc6e631a2e23b9a857 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: check asoc strreset_chunk in sctp_generate_reconf_event | expand |
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.
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.
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>
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!
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,
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(+)