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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 24 Mar 2022 09:22:43 +0300 From: Dan Carpenter <dan.carpenter@...cle.com> To: Eric Dumazet <eric.dumazet@...il.com> Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev <netdev@...r.kernel.org>, Eric Dumazet <edumazet@...gle.com>, 赵子轩 <beraphin@...il.com>, Stoyan Manolov <smanolov@...e.de> Subject: Re: [PATCH net-next] llc: fix netdevice reference leaks in llc_ui_bind() 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. 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
Powered by blists - more mailing lists