| 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
| ||
|
Message-ID: <CAD-N9QU6JXi=W64M-LbfCiiaVTWgbDLd2+U8jkhsUBkuuNbmLA@mail.gmail.com>
Date: Tue, 25 May 2021 22:32:34 +0800
From: 慕冬亮 <mudongliangabcd@...il.com>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: Larry.Finger@...inger.net, florian.c.schilhabel@...glemail.com,
Greg KH <gregkh@...uxfoundation.org>, rkovhaev@...il.com,
straube.linux@...il.com, linux-staging@...ts.linux.dev,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv
On Tue, May 25, 2021 at 7:04 PM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> This is a syzbot warning, right? If so, it needs a Reported-by tag.
Yes, https://syzkaller.appspot.com/bug?id=3a325b8389fc41c1bc94de0f4ac437ed13cce584
>
> I don't really understand why rtl871x_load_fw_cb() calls:
I am also confused about the invocation of rtl871x_load_fw_cb. In my
local reproduction, if the firmware (rtlwifi/rtl8712u.bin) is missing,
the memory leak occurs, otherwise, no memory leak is reported.
However, my testing in the syzbot dashboard shows there is no
invocation of rtl871x_load_fw_cb. If that's the fact, then I don't
quite understand how this memory leak occurred.
I doubt the bug I reproduced in the local environment may be not the
bug in the syzbot dashboard. So I did not add Reported-by tag
>
> usb_put_dev(udev);
> usb_set_intfdata(usb_intf, NULL);
>
> That just seems like a layering violation and it keeps causing bugs and
> I think everything would be simpler if we deleted that. That way we
> could remove both checks for if pnetdev is NULL.
>
The following description of patches is **really** friendly for
newbies like me, especially for rules for allocation and deallocation.
I will keep them in mind when developing patches in the future.
Thanks very much. I really appreciate it,
> [PATCH 10/x] staging: rtl8712: delete bogus clean up code in firmware callback
>
> This clean up is supposed to be done in r871xu_dev_remove().
> Setting the usb USB interface leads to leaks which syzbot detects.
> <stack trace>
> Reported-by: Sysbot blah blah
>
> Patch 10 because their are some other easier patches which could be
> done first. Always do the easiest patch first in case a patch set needs
> to be changed, it's easier to change the latter patches.
>
> Your patch fixes one sysbot warning but then syzbot will complain about
> something else because there are a bunch of other things which need to
> be freed as well. Of course, it's complicated to know which things need
> to be freed and which not because the code is really bad. It's better
> to just re-write the cleanup code and fix everything at once.
>
> I always encourage everyone to follow the "free the most recently
> allocated resource" system of error handling. This is the only
> *systematic* way I know to do error handling. Everything else is
> ad-hoc, impossible to review and has proven bug prone in real life.
>
> The rules are:
> 1) Every function cleans up it's own allocations. Never partially
> allocate things. Never leave the caller to clean up your resources.
> 2) Keep track in your head of the most recently allocated resource.
> Keeping track of just one thing is a lot easier than keeping track of
> everything.
> 3) Use good label names which say what the labels free.
>
> err_free_netdev:
> free_netdev(netdev);
>
> 4) Every allocator function has a mirror free function.
>
> How it works in real life is like this:
>
> int my_probe(...)
> {
> a = alloc();
> if (!a)
> return -ENOMEM;
>
> b = alloc();
> if (!b) {
> ret = -ENOMEM;
> goto free_a; // <-- a is most recent allocation
> }
>
> c = alloc();
> if (!c) {
> ret = -ENOMEM;
> goto free_b;
> }
>
> return 0;
>
> free_b:
> free(b);
> free_a:
> free(a);
>
> return ret;
> }
>
> Then the mirror function basically writes its self. You just have to
> cut and paste the clean up code and add a kfree(c) to the start.
>
> void my_release(...)
> {
> free(c);
> free(b);
> free(a);
> }
>
> Once we move all the frees to the correct place and in the right order
> then it becomes simpler.
>
> drivers/staging/rtl8712/usb_intf.c
> 345 static int r871xu_drv_init(struct usb_interface *pusb_intf,
> 346 const struct usb_device_id *pdid)
> 347 {
> 348 uint status;
>
> Keep status for status = padapter->dvobj_init() but everything else
> should be int ret. Eventually "status" will be deleted.
>
> 349 struct _adapter *padapter = NULL;
> 350 struct dvobj_priv *pdvobjpriv;
> 351 struct net_device *pnetdev;
> 352 struct usb_device *udev;
> 353
> 354 /* In this probe function, O.S. will provide the usb interface pointer
> 355 * to driver. We have to increase the reference count of the usb device
> 356 * structure by using the usb_get_dev function.
> 357 */
> 358 udev = interface_to_usbdev(pusb_intf);
> 359 usb_get_dev(udev);
> ^^^^^^^^^^^^^^^^^
> First resource allocation/thing to be freed. Is this really required?
> I'm not sure. Anyway, it's harmless.
>
> 360 pintf = pusb_intf;
> 361 /* step 1. */
>
> Delete all these step 1,2,3... comments. The authors were trying to
> keep track of what they had allocated but unfortunately writing it down
> did not help them.
>
> 362 pnetdev = r8712_init_netdev();
> 363 if (!pnetdev)
> 364 goto error;
>
> if (!pnetdev) {
> ret = -ENOMEM;
> goto put_dev;
> }
>
> r8712_init_netdev() needs a free function. Right now the free_netdev()
> is hidden in r8712_free_drv_sw() which is the wrong place.
>
> void r8712_free_netdev(struct net_device *dev)
> {
> free_netdev(dev);
> }
>
> "pnetdev" is now our current resource.
>
> [PATCH 6/x] staging: rtl8712: create r8712_free_netdev() function
>
> This is a release function for r8712_init_netdev(). I changed
> the free_netdev() in r871xu_drv_init() to use this and I moved
> the free_netdev() out of r8712_free_drv_sw() and into the
> r871xu_dev_remove() function where it belongs.
>
> 365 padapter = netdev_priv(pnetdev);
> 366 disable_ht_for_spec_devid(pdid, padapter);
> 367 pdvobjpriv = &padapter->dvobjpriv;
> 368 pdvobjpriv->padapter = padapter;
> 369 padapter->dvobjpriv.pusbdev = udev;
> 370 padapter->pusb_intf = pusb_intf;
> 371 usb_set_intfdata(pusb_intf, pnetdev);
> 372 SET_NETDEV_DEV(pnetdev, &pusb_intf->dev);
> 373 pnetdev->dev.type = &wlan_type;
> 374 /* step 2. */
> 375 padapter->dvobj_init = r8712_usb_dvobj_init;
> 376 padapter->dvobj_deinit = r8712_usb_dvobj_deinit;
> 377 padapter->halpriv.hal_bus_init = r8712_usb_hal_bus_init;
> 378 padapter->dvobjpriv.inirp_init = r8712_usb_inirp_init;
> 379 padapter->dvobjpriv.inirp_deinit = r8712_usb_inirp_deinit;
>
> These function pointers are garbage. We should try to move this code
> to calling the functions directly and delete the pointers from the
> struct.
>
> 380 /* step 3.
> 381 * initialize the dvobj_priv
> 382 */
> 383 if (!padapter->dvobj_init) {
> 384 goto error;
>
>
> We set ->dvobj_init on line 375 so it can't be NULL. Delete this.
>
> [PATCH 1/x] staging: rtl8712: delete impossible NULL check
>
> 385 } else {
> 386 status = padapter->dvobj_init(padapter);
> 387 if (status != _SUCCESS)
> 388 goto error;
>
> Since we know that ->dvobj_init is r8712_usb_dvobj_init() then lets call
> that directly.
>
> status = r8712_usb_dvobj_init(padapter);
> if (status != _SUCCESS) {
> ret = -ENOMEM;
> goto free_netdev;
> }
>
> [PATCH 2/x] staging: rtl8712: Get rid of ->dvobj_init/deinit function pointers
>
> The "padapter->dvobj_init/->dvobj_deinit" pointers are not required.
> We can call the functions directly.
>
> This usb_dvobj (dumb name) is now our current resource. Unfortunately
> the r8712_usb_dvobj_deinit() function is an empty stub function. It
> should be:
>
> static void r8712_usb_dvobj_deinit(struct _adapter *padapter)
> {
> r8712_free_io_queue(padapter);
> }
>
> Currently the call to r8712_free_io_queue() is hidden inside the
> r8712_free_drv_sw() function but that's the wrong place for it.
>
> [PATCH 8/x] staging: rtl8712: implement r8712_usb_dvobj_deinit()
>
> The r8712_usb_dvobj_deinit() function is supposed to clean up from
> r8712_usb_dvobj_deinit(). Currently that is open coded in
> r8712_free_drv_sw(), but it should be implemented as a separate
> function and called from r871xu_dev_remove().
>
> 389 }
> 390 /* step 4. */
> 391 status = r8712_init_drv_sw(padapter);
> 392 if (status)
> 393 goto error;
>
> ret = r8712_init_drv_sw(padapter);
> if (ret)
> goto free_usb_dvobj;
>
> The r8712_init_drv_sw() function does not clean up after itself on
> error.
>
> [PATCH 3/x] staging: rtl8712: fix leaks in r8712_init_drv_sw().
>
> The r8712_free_drv_sw() exists but it is not a mirror of the
> r8712_init_drv_sw() function. As we've noticed, it frees some things
> which it should not but it also leaves timers running so presumably
> that leads to a use after free.
>
> [PATCH 9/x] staging: rtl8712: re-write r8712_free_drv_sw()
>
> The r8712_free_drv_sw() should clean up everything allocated in
> r8712_init_drv_sw() but it does not. Some of it was done in
> r8712_stop_drv_timers() and so I have moved it here and deleted
> that code.
>
> PATCH 9 is slightly risky. Be careful not to introduce any double
> frees!
>
> 394 /* step 5. read efuse/eeprom data and get mac_addr */
> 395 {
> 396 int i, offset;
> 397 u8 mac[6];
> 398 u8 tmpU1b, AutoloadFail, eeprom_CustomerID;
> 399 u8 *pdata = padapter->eeprompriv.efuse_eeprom_data;
>
> Declare this at the top. Pull the code in one tab.
>
> [PATCH 4/x] staging: rtl8712: pull code in one tab
>
> 400
> 401 tmpU1b = r8712_read8(padapter, EE_9346CR);/*CR9346*/
> 402
> 403 /* To check system boot selection.*/
> 404 dev_info(&udev->dev, "r8712u: Boot from %s: Autoload %s\n",
> 405 (tmpU1b & _9356SEL) ? "EEPROM" : "EFUSE",
> 406 (tmpU1b & _EEPROM_EN) ? "OK" : "Failed");
> 407
> 408 /* To check autoload success or not.*/
> 409 if (tmpU1b & _EEPROM_EN) {
> 410 AutoloadFail = true;
>
> Put the rest of this if statement after the "AutoloadFail = true;" into
> a separate function. Set AutoloadFail = false at the top of the
> function:
>
> bool AutoloadFail = false;
>
> [PATCH 5/x] staging: rtl8712: move code to a separate function
>
>
> 411 /* The following operations prevent Efuse leakage by
> 412 * turning on 2.5V.
> 413 */
> 414 tmpU1b = r8712_read8(padapter, EFUSE_TEST + 3);
> 415 r8712_write8(padapter, EFUSE_TEST + 3, tmpU1b | 0x80);
> 416 msleep(20);
> 417 r8712_write8(padapter, EFUSE_TEST + 3,
> 418 (tmpU1b & (~BIT(7))));
> 419
> 420 /* Retrieve Chip version.
> 421 * Recognize IC version by Reg0x4 BIT15.
> 422 */
> 423 tmpU1b = (u8)((r8712_read32(padapter, PMC_FSM) >> 15) &
> 424 0x1F);
> 425 if (tmpU1b == 0x3)
> 426 padapter->registrypriv.chip_version =
> 427 RTL8712_3rdCUT;
> 428 else
> 429 padapter->registrypriv.chip_version =
> 429 padapter->registrypriv.chip_version =
> 430 (tmpU1b >> 1) + 1;
> 431 switch (padapter->registrypriv.chip_version) {
> 432 case RTL8712_1stCUT:
> 433 case RTL8712_2ndCUT:
> 434 case RTL8712_3rdCUT:
> 435 break;
> 436 default:
> 437 padapter->registrypriv.chip_version =
> 438 RTL8712_2ndCUT;
> 439 break;
> 440 }
> 441
> 442 for (i = 0, offset = 0; i < 128; i += 8, offset++)
> 443 r8712_efuse_pg_packet_read(padapter, offset,
> 444 &pdata[i]);
> 445
> 446 if (!r8712_initmac || !mac_pton(r8712_initmac, mac)) {
> 447 /* Use the mac address stored in the Efuse
> 448 * offset = 0x12 for usb in efuse
> 449 */
> 450 ether_addr_copy(mac, &pdata[0x12]);
> 451 }
> 452 eeprom_CustomerID = pdata[0x52];
> 453 switch (eeprom_CustomerID) {
> 454 case EEPROM_CID_ALPHA:
> 455 padapter->eeprompriv.CustomerID =
> 456 RT_CID_819x_ALPHA;
> 457 break;
> 458 case EEPROM_CID_CAMEO:
> 459 padapter->eeprompriv.CustomerID =
> 460 RT_CID_819x_CAMEO;
> 461 break;
> 462 case EEPROM_CID_SITECOM:
> 463 padapter->eeprompriv.CustomerID =
> 464 RT_CID_819x_Sitecom;
> 465 break;
> 466 case EEPROM_CID_COREGA:
> 467 padapter->eeprompriv.CustomerID =
> 468 RT_CID_COREGA;
> 469 break;
> 470 case EEPROM_CID_Senao:
> 471 padapter->eeprompriv.CustomerID =
> 472 RT_CID_819x_Senao;
> 473 break;
> 474 case EEPROM_CID_EDIMAX_BELKIN:
> 475 padapter->eeprompriv.CustomerID =
> 476 RT_CID_819x_Edimax_Belkin;
> 477 break;
> 478 case EEPROM_CID_SERCOMM_BELKIN:
> 479 padapter->eeprompriv.CustomerID =
> 480 RT_CID_819x_Sercomm_Belkin;
> 481 break;
> 482 case EEPROM_CID_WNC_COREGA:
> 483 padapter->eeprompriv.CustomerID =
> 484 RT_CID_819x_WNC_COREGA;
> 485 break;
> 486 case EEPROM_CID_WHQL:
> 487 break;
> 488 case EEPROM_CID_NetCore:
> 489 padapter->eeprompriv.CustomerID =
> 490 RT_CID_819x_Netcore;
> 491 break;
> 492 case EEPROM_CID_CAMEO1:
> 493 padapter->eeprompriv.CustomerID =
> 494 RT_CID_819x_CAMEO1;
> 495 break;
> 496 case EEPROM_CID_CLEVO:
> 497 padapter->eeprompriv.CustomerID =
> 498 RT_CID_819x_CLEVO;
> 499 break;
> 500 default:
> 501 padapter->eeprompriv.CustomerID =
> 502 RT_CID_DEFAULT;
> 503 break;
> 504 }
> 505 dev_info(&udev->dev, "r8712u: CustomerID = 0x%.4x\n",
> 506 padapter->eeprompriv.CustomerID);
> 507 /* Led mode */
> 508 switch (padapter->eeprompriv.CustomerID) {
> 509 case RT_CID_DEFAULT:
> 510 case RT_CID_819x_ALPHA:
> 510 case RT_CID_819x_ALPHA:
> 511 case RT_CID_819x_CAMEO:
> 512 padapter->ledpriv.LedStrategy = SW_LED_MODE1;
> 513 padapter->ledpriv.bRegUseLed = true;
> 514 break;
> 515 case RT_CID_819x_Sitecom:
> 516 padapter->ledpriv.LedStrategy = SW_LED_MODE2;
> 517 padapter->ledpriv.bRegUseLed = true;
> 518 break;
> 519 case RT_CID_COREGA:
> 520 case RT_CID_819x_Senao:
> 521 padapter->ledpriv.LedStrategy = SW_LED_MODE3;
> 522 padapter->ledpriv.bRegUseLed = true;
> 523 break;
> 524 case RT_CID_819x_Edimax_Belkin:
> 525 padapter->ledpriv.LedStrategy = SW_LED_MODE4;
> 526 padapter->ledpriv.bRegUseLed = true;
> 527 break;
> 528 case RT_CID_819x_Sercomm_Belkin:
> 529 padapter->ledpriv.LedStrategy = SW_LED_MODE5;
> 530 padapter->ledpriv.bRegUseLed = true;
> 531 break;
> 532 case RT_CID_819x_WNC_COREGA:
> 533 padapter->ledpriv.LedStrategy = SW_LED_MODE6;
> 534 padapter->ledpriv.bRegUseLed = true;
> 535 break;
> 536 default:
> 537 padapter->ledpriv.LedStrategy = SW_LED_MODE0;
> 538 padapter->ledpriv.bRegUseLed = false;
> 539 break;
> 540 }
> 541 } else {
> 542 AutoloadFail = false;
> 543 }
> 544 if (((mac[0] == 0xff) && (mac[1] == 0xff) &&
> 545 (mac[2] == 0xff) && (mac[3] == 0xff) &&
> 546 (mac[4] == 0xff) && (mac[5] == 0xff)) ||
> 547 ((mac[0] == 0x00) && (mac[1] == 0x00) &&
> 548 (mac[2] == 0x00) && (mac[3] == 0x00) &&
> 549 (mac[4] == 0x00) && (mac[5] == 0x00)) ||
> 550 (!AutoloadFail)) {
> 551 mac[0] = 0x00;
> 552 mac[1] = 0xe0;
> 553 mac[2] = 0x4c;
> 554 mac[3] = 0x87;
> 555 mac[4] = 0x00;
> 556 mac[5] = 0x00;
> 557 }
> 558 if (r8712_initmac) {
> 559 /* Make sure the user did not select a multicast
> 560 * address by setting bit 1 of first octet.
> 561 */
> 562 mac[0] &= 0xFE;
> 563 dev_info(&udev->dev,
> 564 "r8712u: MAC Address from user = %pM\n", mac);
> 565 } else {
> 566 dev_info(&udev->dev,
> 567 "r8712u: MAC Address from efuse = %pM\n", mac);
> 568 }
> 569 ether_addr_copy(pnetdev->dev_addr, mac);
> 570 }
> 571 /* step 6. Load the firmware asynchronously */
> 572 if (rtl871x_load_fw(padapter))
> 573 goto error;
>
> The big indent block didn't allocate anything so our most recent
> allocation is still drv_sw.
>
> ret = rtl871x_load_fw(padapter);
> if (ret)
> goto free_drv_sw;
>
> 574 spin_lock_init(&padapter->lock_rx_ff0_filter);
> 575 mutex_init(&padapter->mutex_start);
> 576 return 0;
> 577 error:
> 578 usb_put_dev(udev);
> 579 usb_set_intfdata(pusb_intf, NULL);
> 580 if (padapter && padapter->dvobj_deinit)
> 581 padapter->dvobj_deinit(padapter);
> 582 if (pnetdev)
> 583 free_netdev(pnetdev);
> 584 return -ENODEV;
> 585 }
>
> dvobj_deinit() is a no-op as discussed. This also doesn't release
> the drv_sw. So there are some leaks. When we fix and implement the
> mirrored clean up functions it becomes:
>
> return 0;
>
> free_drv_sw:
> r8712_free_drv_sw(padapter);
> free_usb_dvobj:
> r8712_usb_dvobj_deinit(padapter);
> free_netdev:
> r8712_free_netdev(pnetdev);
> put_dev:
> usb_put_dev(udev);
> usb_set_intfdata(pusb_intf, NULL);
>
> return ret;
> }
>
> Now we can implement a remove function. It's complicated because
> we don't know if the firmware loaded successfully and if the network
> device is registered. We don't really need to test if (adapter->fw)
> becaue release_firmware(NULL) is a no-op but I did it anyway.
>
> Based on what we know so far, this is what it should look like:
>
> static void r871xu_dev_remove(struct usb_interface *pusb_intf)
> {
> struct net_device *pnetdev = usb_get_intfdata(pusb_intf);
> struct usb_device *udev = interface_to_usbdev(pusb_intf);
> struct _adapter *padapter = netdev_priv(pnetdev);
>
> /* never exit with a firmware callback pending */
> wait_for_completion(&padapter->rtl8712_fw_ready);
> if (pnetdev->reg_state != NETREG_UNINITIALIZED)
> unregister_netdev(pnetdev); /* will call netdev_close() */
> if (adapter->fw)
> release_firmware(padapter->fw);
> r8712_free_drv_sw(padapter);
> r8712_usb_dvobj_deinit(padapter);
> r8712_free_netdev(pnetdev);
> usb_put_dev(udev);
> usb_set_intfdata(pusb_intf, NULL);
> }
>
> The kernel's r871xu_dev_remove() look different so there are some
> remaining questions.
>
> 585 static void r871xu_dev_remove(struct usb_interface *pusb_intf)
> 586 {
> 587 struct net_device *pnetdev = usb_get_intfdata(pusb_intf);
> 588 struct usb_device *udev = interface_to_usbdev(pusb_intf);
> 589
> 590 if (pnetdev) {
>
> These checks are no longer required now that we deleted the two lines
> from rtl871x_load_fw_cb().
>
> 591 struct _adapter *padapter = netdev_priv(pnetdev);
> 592
> 593 /* never exit with a firmware callback pending */
> 594 wait_for_completion(&padapter->rtl8712_fw_ready);
> 595 pnetdev = usb_get_intfdata(pusb_intf);
>
> This assignment is not required becuse "pnetdev" remains the same.
>
> 596 usb_set_intfdata(pusb_intf, NULL);
> 597 if (!pnetdev)
> 598 goto firmware_load_fail;
> 599 release_firmware(padapter->fw);
> 600 if (drvpriv.drv_registered)
> 601 padapter->surprise_removed = true;
>
> All the "padapter->surprise_removed" code is bogus, but it needs to
> be audited and deleted before any other fixes to the error handling can
> be done.
>
> [PATCH 7/x] staging: rtl8712: delete padapter->surprise_removed
>
>
> 602 if (pnetdev->reg_state != NETREG_UNINITIALIZED)
> 603 unregister_netdev(pnetdev); /* will call netdev_close() */
> 604 flush_scheduled_work();
> 605 udelay(1);
>
> This is a layering violation. If it's really required then it needs to
> be done in the correct location...
>
> 606 /* Stop driver mlme relation timer */
> 607 r8712_stop_drv_timers(padapter);
>
> Once r8712_free_drv_sw() is fixed I think it will take care of the
> timers.
>
> 608 r871x_dev_unload(padapter);
>
> The r871x_dev_unload() stuff is suposed to be move to netdev_close(), I
> think?
>
> 609 r8712_free_drv_sw(padapter);
> 610
> 611 /* decrease the reference count of the usb device structure
> 612 * when disconnect
> 613 */
>
> Pointless comment.
>
> 614 usb_put_dev(udev);
> 615 }
> 616 firmware_load_fail:
> 617 /* If we didn't unplug usb dongle and remove/insert module, driver
> 618 * fails on sitesurvey for the first time when device is up.
> 619 * Reset usb port for sitesurvey fail issue.
> 620 */
> 621 if (udev->state != USB_STATE_NOTATTACHED)
> 622 usb_reset_device(udev);
>
> This feels wrong. Also it feels like using "udev" after calling
> usb_put_dev(udev) would be a use after free, but I think the stuff
> with usb_get/put_dev() is not really required so it doesn't matter.
>
> Anyway, leave this stuff. Even though, we might not like it we can't
> change it without a way to test it using real hardware. That's the
> same for the flush_scheduled_work() and the udelay(1). We don't like it
> but we can't test it.
>
> 623 }
>
> It would probably take a 15 patch series to fix this code. The ordering
> is important but slightly tricky so be careful with it.
>
> regards,
> dan carpenter
Powered by blists - more mailing lists