| Message ID | 20231023130609.595122-1-dxld@darkboxed.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | wireguard: Fix leaking sockets in wg_socket_init error paths | expand |
Hi, The signed-off-by is missing and the subject does not match the format of any other wireguard commits. On Mon, Oct 23, 2023 at 03:06:09PM +0200, Daniel Gröber wrote: > This doesn't seem to be reachable normally, but while working on a patch "Normally" as in what? At all? Or? > for the address binding code I ended up triggering this leak and had to > reboot to get rid of the leaking wg sockets. This commit message doesn't describe any rationale for this patch. Can you describe the bug? > --- > drivers/net/wireguard/socket.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c > index 0414d7a6ce74..c35163f503e7 100644 > --- a/drivers/net/wireguard/socket.c > +++ b/drivers/net/wireguard/socket.c > @@ -387,7 +387,7 @@ int wg_socket_init(struct wg_device *wg, u16 port) > ret = udp_sock_create(net, &port4, &new4); > if (ret < 0) { > pr_err("%s: Could not create IPv4 socket\n", wg->dev->name); > - goto out; > + goto err; `new4` is either NULL or has already been freed here in the `goto retry` case. `new6` is NULL here. > } > set_sock_opts(new4); > setup_udp_tunnel_sock(net, new4, &cfg); > @@ -402,7 +402,7 @@ int wg_socket_init(struct wg_device *wg, u16 port) > goto retry; > pr_err("%s: Could not create IPv6 socket\n", > wg->dev->name); > - goto out; > + goto err; `new4` has just been freed by `udp_tunnel_sock_release` just above the context. `new6` is NULL. > } > set_sock_opts(new6); > setup_udp_tunnel_sock(net, new6, &cfg); > @@ -414,6 +414,11 @@ int wg_socket_init(struct wg_device *wg, u16 port) > out: > put_net(net); > return ret; > + > +err: > + sock_free(new4 ? new4->sk : NULL); > + sock_free(new6 ? new6->sk : NULL); > + goto out; > } > > void wg_socket_reinit(struct wg_device *wg, struct sock *new4, I don't see the bug. If there is one, maybe try again with a real patch that describes it better. If there isn't one, what is the point? Jason
Hi Jason, On Mon, Oct 23, 2023 at 04:04:13PM +0200, Jason A. Donenfeld wrote: > The signed-off-by is missing and the subject does not match the format > of any other wireguard commits. Ah, I don't usually send kernel patches. Forgot to do format.signOff=true. > On Mon, Oct 23, 2023 at 03:06:09PM +0200, Daniel Gröber wrote: > > This doesn't seem to be reachable normally, but while working on a patch > > "Normally" as in what? At all? Or? I committed this while working on my address/ifindex binding patch[1] (which I will also resend shortly), at the time I thought this fix makes sense in isolation but apparently not. [1]: https://lists.zx2c4.com/pipermail/wireguard/2023-August/008148.html, > > for the address binding code I ended up triggering this leak and had to > > reboot to get rid of the leaking wg sockets. > > This commit message doesn't describe any rationale for this patch. Can > you describe the bug? It's been a while since I wrote this patch. Unfortunately you didn't respond to my initial mail in Aug, so some context has already been lost to time. I may have been under the mistaken impression that udp_sock_create can return <0 while leaving *sockp!=NULL, but as I recall it I did re-test with this patch and it fixed the bug, that I wish I remembered how to trigger now. Unsatisfying. --Daniel
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c index 0414d7a6ce74..c35163f503e7 100644 --- a/drivers/net/wireguard/socket.c +++ b/drivers/net/wireguard/socket.c @@ -387,7 +387,7 @@ int wg_socket_init(struct wg_device *wg, u16 port) ret = udp_sock_create(net, &port4, &new4); if (ret < 0) { pr_err("%s: Could not create IPv4 socket\n", wg->dev->name); - goto out; + goto err; } set_sock_opts(new4); setup_udp_tunnel_sock(net, new4, &cfg); @@ -402,7 +402,7 @@ int wg_socket_init(struct wg_device *wg, u16 port) goto retry; pr_err("%s: Could not create IPv6 socket\n", wg->dev->name); - goto out; + goto err; } set_sock_opts(new6); setup_udp_tunnel_sock(net, new6, &cfg); @@ -414,6 +414,11 @@ int wg_socket_init(struct wg_device *wg, u16 port) out: put_net(net); return ret; + +err: + sock_free(new4 ? new4->sk : NULL); + sock_free(new6 ? new6->sk : NULL); + goto out; } void wg_socket_reinit(struct wg_device *wg, struct sock *new4,