Project

General

Profile

Bug #12905

drv_ioc_prop_common could leak memory and holds

Added by Ryan Zezeski 4 months ago. Updated 4 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

This is an upstream of SmartOS ticket OS-5892.

The drv_ioc_prop_common() function starts by making a kmem allocation and grabbing the dls temp hold, mac perim, and dls hold for the link. If at any point something should fail it sets err and jumps to done where the resources are correctly released -- except for one case. If the caller is performing a read of MAC_PROP_AUTOPUSH and the valsize is 0 then it will directly return without releasing the resources.

            if (kprop->pr_valsize == 0)
                return (ENOBUFS);

If yous study this code you might notice the following check earlier in the function and think that it should catch the case of pr_valsize == 0.

    if (!mac_prop_check_size(kprop->pr_num, kprop->pr_valsize,
        kprop->pr_flags & DLD_PROP_POSSIBLE)) {
        err = ENOBUFS;
        goto done;
    }

However, MAC_PROP_AUTOPUSH uses the size 0 as a sentinel on the write side to clear the prop.

Triggering these leaks, as far as I know, cannot be done with existing commands. Rather you must craft your own direct DLD consumer to exercise them, which is how I produced the leaks demonstrated below. The first leak is concerning, but the second leak is the real worry, as it renders the entire mac system useless until reboot.

> ::findleaks
CACHE             LEAKED           BUFCTL CALLER
fffffe59194b5008       1 fffffe5a74400060 drv_ioc_prop_common+0xbc
------------------------------------------------------------------------
           Total       1 buffer, 320 bytes

> ::pgrep 5892
S PID    PPID   PGID   SID    UID    FLAGS      ADDR             NAME
R 101085 101084 101084 101076 101    0x4a004000 fffffe5a8eb54038 OS-5892

> 0t101085::pid2proc |::walk thread |::stacks
THREAD           STATE    SOBJ                COUNT
fffffe5a6152a800 SLEEP    CV                      1
                 swtch+0x16b
                 cv_wait+0x89
                 i_mac_perim_enter+0x63
                 mac_perim_enter_by_mh+0x23
                 mac_perim_enter_by_macname+0x37
                 drv_ioc_prop_common+0x27b
                 drv_ioc_getprop+0x18
                 drv_ioctl+0x1ef
                 cdev_ioctl+0x2b
                 spec_ioctl+0x45
                 fop_ioctl+0x5b
                 ioctl+0x153
                 _sys_sysenter_post_swapgs+0x24f

> fffffe5a6152a800::findstack -v
stack pointer for thread fffffe5a6152a800 (OS-5892/1): fffffe007d9269a0
[ fffffe007d9269a0 _resume_from_idle+0x12b() ]
  fffffe007d9269d0 swtch+0x16b()
  fffffe007d926a00 cv_wait+0x89(fffffe59f4d3baac, fffffe59f4d3ba98)
  fffffe007d926a40 i_mac_perim_enter+0x63(fffffe59f4d3b9c0)
  fffffe007d926a70 mac_perim_enter_by_mh+0x23(fffffe59f4d3b9c0, fffffe007d926b20)
  fffffe007d926ac0 mac_perim_enter_by_macname+0x37(fffffe59eceb492c, fffffe007d926b20)
  fffffe007d926b90 drv_ioc_prop_common+0x27b(fffffe5a5e68f300, 8d8f410, 0, fffffe59e7c3dde0, 100003)
  fffffe007d926bd0 drv_ioc_getprop+0x18(fffffe5a5e68f300, 8d8f410, 100003, fffffe59e7c3dde0, fffffe007d926dc8)
  fffffe007d926c70 drv_ioctl+0x1ef(3500000000, d1d001c, 8d8f410, 100003, fffffe59e7c3dde0, fffffe007d926dc8)
  fffffe007d926cb0 cdev_ioctl+0x2b(3500000000, d1d001c, 8d8f410, 100003, fffffe59e7c3dde0, fffffe007d926dc8)
  fffffe007d926d00 spec_ioctl+0x45(fffffe59df91f000, d1d001c, 8d8f410, 100003, fffffe59e7c3dde0, fffffe007d926dc8, 0)
  fffffe007d926d90 fop_ioctl+0x5b(fffffe59df91f000, d1d001c, 8d8f410, 100003, fffffe59e7c3dde0, fffffe007d926dc8, 0)
  fffffe007d926eb0 ioctl+0x153(3, d1d001c, 8d8f410)
  fffffe007d926f00 _sys_sysenter_post_swapgs+0x24f()

> fffffe59f4d3b9c0::print mac_impl_t mi_perim_owner |::thread
            ADDR    STATE  FLG PFLG SFLG   PRI  EPRI PIL             INTR
fffffe5a5fd3ab60 inval/deadbeef beef dead beef -16657 -8531 173 deadbeefdeadbeef
#1

Updated by Electric Monk 4 months ago

  • Gerrit CR set to 760
#2

Updated by Ryan Zezeski 4 months ago

I tested this by running my custom DLD consumer and verifying that the mac perim is not leaked and that a forced dump on DEBUG kernel reports nothing for ::findleaks.

#3

Updated by Electric Monk 4 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit daa7e8a345b2e0424e612017e8ead2e97b0f7f37

commit  daa7e8a345b2e0424e612017e8ead2e97b0f7f37
Author: Ryan Zezeski <rpz@joyent.com>
Date:   2020-07-02T01:41:41.000Z

    12905 drv_ioc_prop_common could leak memory and holds
    Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
    Reviewed by: Igor Kozhukhov <igor@dilos.org>
    Reviewed by: Paul Winder <paul@winders.demon.co.uk>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Approved by: Robert Mustacchi <rm@fingolfin.org>

Also available in: Atom PDF