netdev - Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKJamk6v5gt67tE0tG0i3XS2LofJu34uT=_AVqYCs-0SQ@mail.gmail.com>
Date:   Thu, 24 Mar 2022 07:38:42 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        赵子轩 <beraphin@...il.com>,
        Stoyan Manolov <smanolov@...e.de>
Subject: Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind()

On Wed, Mar 23, 2022 at 11:23 PM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> On Tue, Mar 22, 2022 at 05:41:47PM -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@...gle.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@...gle.com>
> > Reported-by: 赵子轩 <beraphin@...il.com>
> > Reported-by: Stoyan Manolov <smanolov@...e.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