| Message ID | a93e655b3c153dc8945d7a812e6d8ab0d52b7aa0.1727729391.git.lucien.xin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8beee4d8dee76b67c75dc91fd8185d91e845c160 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: set sk_state back to CLOSED if autobind fails in sctp_listen_start | expand |
On Mon, Sep 30, 2024 at 04:49:51PM -0400, Xin Long wrote: > In sctp_listen_start() invoked by sctp_inet_listen(), it should set the > sk_state back to CLOSED if sctp_autobind() fails due to whatever reason. > > Otherwise, next time when calling sctp_inet_listen(), if sctp_sk(sk)->reuse > is already set via setsockopt(SCTP_REUSE_PORT), sctp_sk(sk)->bind_hash will > be dereferenced as sk_state is LISTENING, which causes a crash as bind_hash > is NULL. > > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > RIP: 0010:sctp_inet_listen+0x7f0/0xa20 net/sctp/socket.c:8617 > Call Trace: > <TASK> > __sys_listen_socket net/socket.c:1883 [inline] > __sys_listen+0x1b7/0x230 net/socket.c:1894 > __do_sys_listen net/socket.c:1902 [inline] > > Fixes: 5e8f3f703ae4 ("sctp: simplify sctp listening code") > Reported-by: syzbot+f4e0f821e3a3b7cee51d@syzkaller.appspotmail.com > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/sctp/socket.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 32f76f1298da..078bcb3858c7 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -8557,8 +8557,10 @@ static int sctp_listen_start(struct sock *sk, int backlog) > */ > inet_sk_set_state(sk, SCTP_SS_LISTENING); > if (!ep->base.bind_addr.port) { > - if (sctp_autobind(sk)) > + if (sctp_autobind(sk)) { > + inet_sk_set_state(sk, SCTP_SS_CLOSED); > return -EAGAIN; > + } > } else { > if (sctp_get_port(sk, inet_sk(sk)->inet_num)) { > inet_sk_set_state(sk, SCTP_SS_CLOSED); Then AFAICT the end of the function needs a patch as well, because it returns what could be an error directly, without undoing this: WRITE_ONCE(sk->sk_max_ack_backlog, backlog); return sctp_hash_endpoint(ep); }
On Tue, Oct 1, 2024 at 3:15 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Mon, Sep 30, 2024 at 04:49:51PM -0400, Xin Long wrote: > > In sctp_listen_start() invoked by sctp_inet_listen(), it should set the > > sk_state back to CLOSED if sctp_autobind() fails due to whatever reason. > > > > Otherwise, next time when calling sctp_inet_listen(), if sctp_sk(sk)->reuse > > is already set via setsockopt(SCTP_REUSE_PORT), sctp_sk(sk)->bind_hash will > > be dereferenced as sk_state is LISTENING, which causes a crash as bind_hash > > is NULL. > > > > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > > RIP: 0010:sctp_inet_listen+0x7f0/0xa20 net/sctp/socket.c:8617 > > Call Trace: > > <TASK> > > __sys_listen_socket net/socket.c:1883 [inline] > > __sys_listen+0x1b7/0x230 net/socket.c:1894 > > __do_sys_listen net/socket.c:1902 [inline] > > > > Fixes: 5e8f3f703ae4 ("sctp: simplify sctp listening code") > > Reported-by: syzbot+f4e0f821e3a3b7cee51d@syzkaller.appspotmail.com > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > net/sctp/socket.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index 32f76f1298da..078bcb3858c7 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -8557,8 +8557,10 @@ static int sctp_listen_start(struct sock *sk, int backlog) > > */ > > inet_sk_set_state(sk, SCTP_SS_LISTENING); > > if (!ep->base.bind_addr.port) { > > - if (sctp_autobind(sk)) > > + if (sctp_autobind(sk)) { > > + inet_sk_set_state(sk, SCTP_SS_CLOSED); > > return -EAGAIN; > > + } > > } else { > > if (sctp_get_port(sk, inet_sk(sk)->inet_num)) { > > inet_sk_set_state(sk, SCTP_SS_CLOSED); > > Then AFAICT the end of the function needs a patch as well, because it > returns what could be an error directly, without undoing this: > > WRITE_ONCE(sk->sk_max_ack_backlog, backlog); > return sctp_hash_endpoint(ep); > } > Right, but this is another issue and won't cause a crash, and the fix will need a different "Fixes:". I think we should fix it in a separate patch.
On Tue, Oct 01, 2024 at 03:41:23PM -0400, Xin Long wrote: > On Tue, Oct 1, 2024 at 3:15 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Mon, Sep 30, 2024 at 04:49:51PM -0400, Xin Long wrote: > > > In sctp_listen_start() invoked by sctp_inet_listen(), it should set the > > > sk_state back to CLOSED if sctp_autobind() fails due to whatever reason. > > > > > > Otherwise, next time when calling sctp_inet_listen(), if sctp_sk(sk)->reuse > > > is already set via setsockopt(SCTP_REUSE_PORT), sctp_sk(sk)->bind_hash will > > > be dereferenced as sk_state is LISTENING, which causes a crash as bind_hash > > > is NULL. > > > > > > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] > > > RIP: 0010:sctp_inet_listen+0x7f0/0xa20 net/sctp/socket.c:8617 > > > Call Trace: > > > <TASK> > > > __sys_listen_socket net/socket.c:1883 [inline] > > > __sys_listen+0x1b7/0x230 net/socket.c:1894 > > > __do_sys_listen net/socket.c:1902 [inline] > > > > > > Fixes: 5e8f3f703ae4 ("sctp: simplify sctp listening code") > > > Reported-by: syzbot+f4e0f821e3a3b7cee51d@syzkaller.appspotmail.com > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > net/sctp/socket.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > index 32f76f1298da..078bcb3858c7 100644 > > > --- a/net/sctp/socket.c > > > +++ b/net/sctp/socket.c > > > @@ -8557,8 +8557,10 @@ static int sctp_listen_start(struct sock *sk, int backlog) > > > */ > > > inet_sk_set_state(sk, SCTP_SS_LISTENING); > > > if (!ep->base.bind_addr.port) { > > > - if (sctp_autobind(sk)) > > > + if (sctp_autobind(sk)) { > > > + inet_sk_set_state(sk, SCTP_SS_CLOSED); > > > return -EAGAIN; > > > + } > > > } else { > > > if (sctp_get_port(sk, inet_sk(sk)->inet_num)) { > > > inet_sk_set_state(sk, SCTP_SS_CLOSED); > > > > Then AFAICT the end of the function needs a patch as well, because it > > returns what could be an error directly, without undoing this: > > > > WRITE_ONCE(sk->sk_max_ack_backlog, backlog); > > return sctp_hash_endpoint(ep); > > } > > > Right, but this is another issue and won't cause a crash, and the fix will > need a different "Fixes:". I think we should fix it in a separate patch. Makes sense. Thx. Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Mon, 30 Sep 2024 16:49:51 -0400 you wrote: > In sctp_listen_start() invoked by sctp_inet_listen(), it should set the > sk_state back to CLOSED if sctp_autobind() fails due to whatever reason. > > Otherwise, next time when calling sctp_inet_listen(), if sctp_sk(sk)->reuse > is already set via setsockopt(SCTP_REUSE_PORT), sctp_sk(sk)->bind_hash will > be dereferenced as sk_state is LISTENING, which causes a crash as bind_hash > is NULL. > > [...] Here is the summary with links: - [net] sctp: set sk_state back to CLOSED if autobind fails in sctp_listen_start https://git.kernel.org/netdev/net/c/8beee4d8dee7 You are awesome, thank you!
diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 32f76f1298da..078bcb3858c7 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -8557,8 +8557,10 @@ static int sctp_listen_start(struct sock *sk, int backlog) */ inet_sk_set_state(sk, SCTP_SS_LISTENING); if (!ep->base.bind_addr.port) { - if (sctp_autobind(sk)) + if (sctp_autobind(sk)) { + inet_sk_set_state(sk, SCTP_SS_CLOSED); return -EAGAIN; + } } else { if (sctp_get_port(sk, inet_sk(sk)->inet_num)) { inet_sk_set_state(sk, SCTP_SS_CLOSED);
In sctp_listen_start() invoked by sctp_inet_listen(), it should set the sk_state back to CLOSED if sctp_autobind() fails due to whatever reason. Otherwise, next time when calling sctp_inet_listen(), if sctp_sk(sk)->reuse is already set via setsockopt(SCTP_REUSE_PORT), sctp_sk(sk)->bind_hash will be dereferenced as sk_state is LISTENING, which causes a crash as bind_hash is NULL. KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007] RIP: 0010:sctp_inet_listen+0x7f0/0xa20 net/sctp/socket.c:8617 Call Trace: <TASK> __sys_listen_socket net/socket.c:1883 [inline] __sys_listen+0x1b7/0x230 net/socket.c:1894 __do_sys_listen net/socket.c:1902 [inline] Fixes: 5e8f3f703ae4 ("sctp: simplify sctp listening code") Reported-by: syzbot+f4e0f821e3a3b7cee51d@syzkaller.appspotmail.com Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/socket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)