[PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[Eric Dumazet]

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.

 net/llc/af_llc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

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;
 }
-- 
2.35.1.894.gb6a874cedc-goog

Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[Dan Carpenter]

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

Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[Eric Dumazet]

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

Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

[Eric Dumazet]

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.