| Message ID | 20220323004147.1990845-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 764f4eb6846f5475f1244767d24d25dd86528a4a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] llc: fix netdevice reference leaks in llc_ui_bind() | expand |
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 22 Mar 2022 17:41:47 -0700 you wrote: > From: Eric Dumazet <edumazet@google.com> > > Whenever llc_ui_bind() and/or llc_ui_autobind() > took a reference on a netdevice but subsequently fail, > they must properly release their reference > or risk the infamous message from unregister_netdevice() > at device dismantle. > > [...] Here is the summary with links: - [net-next] llc: fix netdevice reference leaks in llc_ui_bind() https://git.kernel.org/netdev/net-next/c/764f4eb6846f You are awesome, thank you!
On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > Whenever llc_ui_bind() and/or llc_ui_autobind() > took a reference on a netdevice but subsequently fail, > they must properly release their reference > or risk the infamous message from unregister_netdevice() > at device dismantle. > > unregister_netdevice: waiting for eth0 to become free. Usage count = 3 > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: 赵子轩 <beraphin@gmail.com> > Reported-by: Stoyan Manolov <smanolov@suse.de> > --- > > This can be applied on net tree, depending on how network maintainers > plan to push the fix to Linus, this is obviously a stable candidate. This patch is fine, but it's that function is kind of ugly and difficult for static analysis to parse. net/llc/af_llc.c 274 static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr) 275 { 276 struct sock *sk = sock->sk; 277 struct llc_sock *llc = llc_sk(sk); 278 struct llc_sap *sap; 279 int rc = -EINVAL; 280 281 if (!sock_flag(sk, SOCK_ZAPPED)) 282 goto out; This condition is checking to see if someone else already initialized llc->dev. If we call dev_put_track(llc->dev, &llc->dev_tracker) on something we didn't allocate then it leads to a use after free. But fortunately the callers all check SOCK_ZAPPED so the condition is impossible. 283 if (!addr->sllc_arphrd) 284 addr->sllc_arphrd = ARPHRD_ETHER; 285 if (addr->sllc_arphrd != ARPHRD_ETHER) 286 goto out; Thus we know that "llc->dev" is NULL on these first couple gotos and calling dev_put_track(llc->dev, &llc->dev_tracker); is a no-op so it's fine. But complicated to review. 287 rc = -ENODEV; 288 if (sk->sk_bound_dev_if) { 289 llc->dev = dev_get_by_index(&init_net, sk->sk_bound_dev_if); 290 if (llc->dev && addr->sllc_arphrd != llc->dev->type) { 291 dev_put(llc->dev); 292 llc->dev = NULL; 293 } 294 } else 295 llc->dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd); 296 if (!llc->dev) 297 goto out; 298 netdev_tracker_alloc(llc->dev, &llc->dev_tracker, GFP_KERNEL); 299 rc = -EUSERS; 300 llc->laddr.lsap = llc_ui_autoport(); 301 if (!llc->laddr.lsap) 302 goto out; 303 rc = -EBUSY; /* some other network layer is using the sap */ 304 sap = llc_sap_open(llc->laddr.lsap, NULL); 305 if (!sap) 306 goto out; 307 memcpy(llc->laddr.mac, llc->dev->dev_addr, IFHWADDRLEN); 308 memcpy(&llc->addr, addr, sizeof(llc->addr)); 309 /* assign new connection to its SAP */ 310 llc_sap_add_socket(sap, sk); 311 sock_reset_flag(sk, SOCK_ZAPPED); 312 rc = 0; 313 out: 314 if (rc) { 315 dev_put_track(llc->dev, &llc->dev_tracker); 316 llc->dev = NULL; 317 } 318 return rc; 319 } regards, dan carpenter
On Wed, Mar 23, 2022 at 11:23 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > Whenever llc_ui_bind() and/or llc_ui_autobind() > > took a reference on a netdevice but subsequently fail, > > they must properly release their reference > > or risk the infamous message from unregister_netdevice() > > at device dismantle. > > > > unregister_netdevice: waiting for eth0 to become free. Usage count = 3 > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: 赵子轩 <beraphin@gmail.com> > > Reported-by: Stoyan Manolov <smanolov@suse.de> > > --- > > > > This can be applied on net tree, depending on how network maintainers > > plan to push the fix to Linus, this is obviously a stable candidate. > > This patch is fine, but it's that function is kind of ugly and difficult > for static analysis to parse. We usually do not mix bug fixes and code refactoring. Please feel free to send a refactor when net-next reopens in two weeks. Thanks. > > net/llc/af_llc.c > 274 static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr) > 275 { > 276 struct sock *sk = sock->sk; > 277 struct llc_sock *llc = llc_sk(sk); > 278 struct llc_sap *sap; > 279 int rc = -EINVAL; > 280 > 281 if (!sock_flag(sk, SOCK_ZAPPED)) > 282 goto out; > > This condition is checking to see if someone else already initialized > llc->dev. If we call dev_put_track(llc->dev, &llc->dev_tracker) on > something we didn't allocate then it leads to a use after free. But > fortunately the callers all check SOCK_ZAPPED so the condition is > impossible. > > 283 if (!addr->sllc_arphrd) > 284 addr->sllc_arphrd = ARPHRD_ETHER; > 285 if (addr->sllc_arphrd != ARPHRD_ETHER) > 286 goto out; > > Thus we know that "llc->dev" is NULL on these first couple gotos and > calling dev_put_track(llc->dev, &llc->dev_tracker); is a no-op so it's > fine. > > But complicated to review. > > 287 rc = -ENODEV; > 288 if (sk->sk_bound_dev_if) { > 289 llc->dev = dev_get_by_index(&init_net, sk->sk_bound_dev_if); > 290 if (llc->dev && addr->sllc_arphrd != llc->dev->type) { > 291 dev_put(llc->dev); > 292 llc->dev = NULL; > 293 } > 294 } else > 295 llc->dev = dev_getfirstbyhwtype(&init_net, addr->sllc_arphrd); > 296 if (!llc->dev) > 297 goto out; > 298 netdev_tracker_alloc(llc->dev, &llc->dev_tracker, GFP_KERNEL); > 299 rc = -EUSERS; > 300 llc->laddr.lsap = llc_ui_autoport(); > 301 if (!llc->laddr.lsap) > 302 goto out; > 303 rc = -EBUSY; /* some other network layer is using the sap */ > 304 sap = llc_sap_open(llc->laddr.lsap, NULL); > 305 if (!sap) > 306 goto out; > 307 memcpy(llc->laddr.mac, llc->dev->dev_addr, IFHWADDRLEN); > 308 memcpy(&llc->addr, addr, sizeof(llc->addr)); > 309 /* assign new connection to its SAP */ > 310 llc_sap_add_socket(sap, sk); > 311 sock_reset_flag(sk, SOCK_ZAPPED); > 312 rc = 0; > 313 out: > 314 if (rc) { > 315 dev_put_track(llc->dev, &llc->dev_tracker); > 316 llc->dev = NULL; > 317 } > 318 return rc; > 319 } > > regards, > dan carpenter
On Thu, Mar 24, 2022 at 7:38 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Mar 23, 2022 at 11:23 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote: > > > From: Eric Dumazet <edumazet@google.com> > > > > > > Whenever llc_ui_bind() and/or llc_ui_autobind() > > > took a reference on a netdevice but subsequently fail, > > > they must properly release their reference > > > or risk the infamous message from unregister_netdevice() > > > at device dismantle. > > > > > > unregister_netdevice: waiting for eth0 to become free. Usage count = 3 > > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > > Reported-by: 赵子轩 <beraphin@gmail.com> > > > Reported-by: Stoyan Manolov <smanolov@suse.de> > > > --- > > > > > > This can be applied on net tree, depending on how network maintainers > > > plan to push the fix to Linus, this is obviously a stable candidate. > > > > This patch is fine, but it's that function is kind of ugly and difficult > > for static analysis to parse. > > We usually do not mix bug fixes and code refactoring. > > Please feel free to send a refactor when net-next reopens in two weeks. > > Thanks. I took another look at this code, and there might be an issue in llc_ui_bind(), if the "goto out;" is taken _before_ we took a reference on a device. We might now release the reference taken by a prior (and successful bind) So it turns out another fix is needed.
diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c index 26c00ebf4fbae4d7dc1c27d180385470fa252be0..c86256064743523f0621f21d5d023956cf1df9a0 100644 --- a/net/llc/af_llc.c +++ b/net/llc/af_llc.c @@ -311,6 +311,10 @@ static int llc_ui_autobind(struct socket *sock, struct sockaddr_llc *addr) sock_reset_flag(sk, SOCK_ZAPPED); rc = 0; out: + if (rc) { + dev_put_track(llc->dev, &llc->dev_tracker); + llc->dev = NULL; + } return rc; } @@ -408,6 +412,10 @@ static int llc_ui_bind(struct socket *sock, struct sockaddr *uaddr, int addrlen) out_put: llc_sap_put(sap); out: + if (rc) { + dev_put_track(llc->dev, &llc->dev_tracker); + llc->dev = NULL; + } release_sock(sk); return rc; }