| Message ID | 9fcd182f1099f86c6661f3717f63712ddd1c676c.1674496737.git.marcelo.leitner@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 458e279f861d3f61796894cd158b780765a1569f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sctp: fail if no bound addresses can be used for a given scope | expand |
On Mon, Jan 23, 2023 at 1:00 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > Currently, if you bind the socket to something like: > servaddr.sin6_family = AF_INET6; > servaddr.sin6_port = htons(0); > servaddr.sin6_scope_id = 0; > inet_pton(AF_INET6, "::1", &servaddr.sin6_addr); > > And then request a connect to: > connaddr.sin6_family = AF_INET6; > connaddr.sin6_port = htons(20000); > connaddr.sin6_scope_id = if_nametoindex("lo"); > inet_pton(AF_INET6, "fe88::1", &connaddr.sin6_addr); > > What the stack does is: > - bind the socket > - create a new asoc > - to handle the connect > - copy the addresses that can be used for the given scope > - try to connect > > But the copy returns 0 addresses, and the effect is that it ends up > trying to connect as if the socket wasn't bound, which is not the > desired behavior. This unexpected behavior also allows KASLR leaks > through SCTP diag interface. > > The fix here then is, if when trying to copy the addresses that can > be used for the scope used in connect() it returns 0 addresses, bail > out. This is what TCP does with a similar reproducer. > > Reported-by: Pietro Borrello <borrello@diag.uniroma1.it> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > net/sctp/bind_addr.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c > index 59e653b528b1faec6c6fcf73f0dd42633880e08d..6b95d3ba8fe1cecf4d75956bf87546b1f1a81c4f 100644 > --- a/net/sctp/bind_addr.c > +++ b/net/sctp/bind_addr.c > @@ -73,6 +73,12 @@ int sctp_bind_addr_copy(struct net *net, struct sctp_bind_addr *dest, > } > } > > + /* If somehow no addresses were found that can be used with this > + * scope, it's an error. > + */ > + if (list_empty(&dest->address_list)) > + error = -ENETUNREACH; > + > out: > if (error) > sctp_bind_addr_clean(dest); > -- > 2.39.0 > Reviewed-by: Xin Long <lucien.xin@gmail.com>
On Mon, 23 Jan 2023 14:59:33 -0300 Marcelo Ricardo Leitner wrote: > Currently, if you bind the socket to something like: > servaddr.sin6_family = AF_INET6; > servaddr.sin6_port = htons(0); > servaddr.sin6_scope_id = 0; > inet_pton(AF_INET6, "::1", &servaddr.sin6_addr); > > And then request a connect to: > connaddr.sin6_family = AF_INET6; > connaddr.sin6_port = htons(20000); > connaddr.sin6_scope_id = if_nametoindex("lo"); > inet_pton(AF_INET6, "fe88::1", &connaddr.sin6_addr); > > What the stack does is: > - bind the socket > - create a new asoc > - to handle the connect > - copy the addresses that can be used for the given scope > - try to connect > > But the copy returns 0 addresses, and the effect is that it ends up > trying to connect as if the socket wasn't bound, which is not the > desired behavior. This unexpected behavior also allows KASLR leaks > through SCTP diag interface. > > The fix here then is, if when trying to copy the addresses that can > be used for the scope used in connect() it returns 0 addresses, bail > out. This is what TCP does with a similar reproducer. > > Reported-by: Pietro Borrello <borrello@diag.uniroma1.it> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Fixes tag?
On Tue, Jan 24, 2023 at 06:14:16PM -0800, Jakub Kicinski wrote: > On Mon, 23 Jan 2023 14:59:33 -0300 Marcelo Ricardo Leitner wrote: > > Currently, if you bind the socket to something like: > > servaddr.sin6_family = AF_INET6; > > servaddr.sin6_port = htons(0); > > servaddr.sin6_scope_id = 0; > > inet_pton(AF_INET6, "::1", &servaddr.sin6_addr); > > > > And then request a connect to: > > connaddr.sin6_family = AF_INET6; > > connaddr.sin6_port = htons(20000); > > connaddr.sin6_scope_id = if_nametoindex("lo"); > > inet_pton(AF_INET6, "fe88::1", &connaddr.sin6_addr); > > > > What the stack does is: > > - bind the socket > > - create a new asoc > > - to handle the connect > > - copy the addresses that can be used for the given scope > > - try to connect > > > > But the copy returns 0 addresses, and the effect is that it ends up > > trying to connect as if the socket wasn't bound, which is not the > > desired behavior. This unexpected behavior also allows KASLR leaks > > through SCTP diag interface. > > > > The fix here then is, if when trying to copy the addresses that can > > be used for the scope used in connect() it returns 0 addresses, bail > > out. This is what TCP does with a similar reproducer. > > > > Reported-by: Pietro Borrello <borrello@diag.uniroma1.it> > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > Fixes tag? Lost in Narnia again, I suppose. :) Ok, I had forgot it, but now checking, it predates git. What should I have used in this case again please? Perhaps just: Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
On Tue, 24 Jan 2023 23:30:22 -0300 Marcelo Ricardo Leitner wrote: > Lost in Narnia again, I suppose. :) :D > Ok, I had forgot it, but now checking, it predates git. > What should I have used in this case again please? Perhaps just: > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Yup, just slap the initial commit ID on it. Let me add when applying.
On Tue, Jan 24, 2023 at 06:32:11PM -0800, Jakub Kicinski wrote: > On Tue, 24 Jan 2023 23:30:22 -0300 Marcelo Ricardo Leitner wrote: > > Lost in Narnia again, I suppose. :) > > :D > > > Ok, I had forgot it, but now checking, it predates git. > > What should I have used in this case again please? Perhaps just: > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Yup, just slap the initial commit ID on it. Let me add when applying. Ok. Thanks!
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Mon, 23 Jan 2023 14:59:33 -0300 you wrote: > Currently, if you bind the socket to something like: > servaddr.sin6_family = AF_INET6; > servaddr.sin6_port = htons(0); > servaddr.sin6_scope_id = 0; > inet_pton(AF_INET6, "::1", &servaddr.sin6_addr); > > And then request a connect to: > connaddr.sin6_family = AF_INET6; > connaddr.sin6_port = htons(20000); > connaddr.sin6_scope_id = if_nametoindex("lo"); > inet_pton(AF_INET6, "fe88::1", &connaddr.sin6_addr); > > [...] Here is the summary with links: - [net] sctp: fail if no bound addresses can be used for a given scope https://git.kernel.org/netdev/net/c/458e279f861d You are awesome, thank you!
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c index 59e653b528b1faec6c6fcf73f0dd42633880e08d..6b95d3ba8fe1cecf4d75956bf87546b1f1a81c4f 100644 --- a/net/sctp/bind_addr.c +++ b/net/sctp/bind_addr.c @@ -73,6 +73,12 @@ int sctp_bind_addr_copy(struct net *net, struct sctp_bind_addr *dest, } } + /* If somehow no addresses were found that can be used with this + * scope, it's an error. + */ + if (list_empty(&dest->address_list)) + error = -ENETUNREACH; + out: if (error) sctp_bind_addr_clean(dest);
Currently, if you bind the socket to something like: servaddr.sin6_family = AF_INET6; servaddr.sin6_port = htons(0); servaddr.sin6_scope_id = 0; inet_pton(AF_INET6, "::1", &servaddr.sin6_addr); And then request a connect to: connaddr.sin6_family = AF_INET6; connaddr.sin6_port = htons(20000); connaddr.sin6_scope_id = if_nametoindex("lo"); inet_pton(AF_INET6, "fe88::1", &connaddr.sin6_addr); What the stack does is: - bind the socket - create a new asoc - to handle the connect - copy the addresses that can be used for the given scope - try to connect But the copy returns 0 addresses, and the effect is that it ends up trying to connect as if the socket wasn't bound, which is not the desired behavior. This unexpected behavior also allows KASLR leaks through SCTP diag interface. The fix here then is, if when trying to copy the addresses that can be used for the scope used in connect() it returns 0 addresses, bail out. This is what TCP does with a similar reproducer. Reported-by: Pietro Borrello <borrello@diag.uniroma1.it> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- net/sctp/bind_addr.c | 6 ++++++ 1 file changed, 6 insertions(+)