Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv

From: Dan Carpenter
Date: Tue May 25 2021 - 07:05:03 EST

Next message: Lukasz Luba: "Re: [PATCH v2 3/3] PM / EM: Skip inefficient OPPs"
Previous message: Jiapeng Chong: "[PATCH] bnx2x: Fix missing error code in bnx2x_iov_init_one()"
In reply to: Dongliang Mu: "[PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv"
Next in thread: 慕冬亮: "Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

This is a syzbot warning, right? If so, it needs a Reported-by tag.

I don't really understand why rtl871x_load_fw_cb() calls:

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.

[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

Next message: Lukasz Luba: "Re: [PATCH v2 3/3] PM / EM: Skip inefficient OPPs"
Previous message: Jiapeng Chong: "[PATCH] bnx2x: Fix missing error code in bnx2x_iov_init_one()"
In reply to: Dongliang Mu: "[PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv"
Next in thread: 慕冬亮: "Re: [PATCH] staging: rtl8712: Fix memory leak in r8712_init_recv_priv"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]