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

[Dan Carpenter <dan.carpenter@oracle.com>]

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