lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Aug 2022 15:26:51 +0200 From: Jakub Sitnicki <jakub@...udflare.com> To: Tom Parkin <tparkin@...alix.com> Cc: netdev@...r.kernel.org, kernel-team@...udflare.com, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Haowei Yan <g1042620637@...il.com> Subject: Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock On Mon, Aug 15, 2022 at 02:21 PM +01, Tom Parkin wrote: > [[PGP Signed Part:Undecided]] > On Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote: >> sk->sk_user_data has multiple users, which are not compatible with each >> other. To synchronize the users, any check-if-unused-and-set access to the >> pointer has to happen with sock lock held. >> >> l2tp currently fails to grab the lock when modifying the underlying tunnel >> socket. Fix it by adding appropriate locking. >> >> We don't to grab the lock when l2tp clears sk_user_data, because it happens >> only in sk->sk_destruct, when the sock is going away. >> >> v2: >> - update Fixes to point to origin of the bug >> - use real names in Reported/Tested-by tags >> >> Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core") > > This still seems wrong to me. > > In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with > lock_sock held. I think you are referring to the PPP-over-L2TP socket, not the UDP socket. In pppol2tp_prepare_tunnel_socket() @ 3557baabf280 we're not holding the sock lock over the UDP socket, AFAICT. > >> Reported-by: Haowei Yan <g1042620637@...il.com> >> Tested-by: Haowei Yan <g1042620637@...il.com> >> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com> >> --- >> Cc: Tom Parkin <tparkin@...alix.com> >> >> net/l2tp/l2tp_core.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c >> index 7499c51b1850..9f5f86bfc395 100644 >> --- a/net/l2tp/l2tp_core.c >> +++ b/net/l2tp/l2tp_core.c >> @@ -1469,16 +1469,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, >> sock = sockfd_lookup(tunnel->fd, &ret); >> if (!sock) >> goto err; >> - >> - ret = l2tp_validate_socket(sock->sk, net, tunnel->encap); >> - if (ret < 0) >> - goto err_sock; >> } >> >> + sk = sock->sk; >> + lock_sock(sk); >> + >> + ret = l2tp_validate_socket(sk, net, tunnel->encap); >> + if (ret < 0) >> + goto err_sock; >> + >> tunnel->l2tp_net = net; >> pn = l2tp_pernet(net); >> >> - sk = sock->sk; >> sock_hold(sk); >> tunnel->sock = sk; >> >> @@ -1504,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, >> >> setup_udp_tunnel_sock(net, sock, &udp_cfg); >> } else { >> - sk->sk_user_data = tunnel; >> + rcu_assign_sk_user_data(sk, tunnel); >> } >> >> tunnel->old_sk_destruct = sk->sk_destruct; >> @@ -1518,6 +1520,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, >> if (tunnel->fd >= 0) >> sockfd_put(sock); >> >> + release_sock(sk); >> return 0; >> >> err_sock: >> @@ -1525,6 +1528,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, >> sock_release(sock); >> else >> sockfd_put(sock); >> + >> + release_sock(sk); >> err: >> return ret; >> } >> -- >> 2.35.3 >>
Powered by blists - more mailing lists