Project

General

Profile

Actions
Copy link

Bug #15305

closed

mac_drop_chain calls freemsg with non-singleton chain

Added by Thirteen Oxide 5 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Luqman Aden
Category:
networking
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:
2591
External Bug:

Description

mac_drop_chain:

        for (mblk_t *mp = chain, *next; mp != NULL; ) {
                next = mp->b_next;
                DTRACE_PROBE2(mac__drop, mblk_t *, mp, char *, msgp);
                freemsg(mp);
                mp = next;
        }

freemsg:

        while (mp) {
                ...
                ASSERT(mp->b_next == NULL && mp->b_prev == NULL);
                ...
        }

So we can call freemsg with the entire chain (b_next != NULL) which is very clearly not what it expects. It's surprising that this doesn't blow up often on DEBUG bits. We found this by inspection while debugging some calling code in non-DEBUG bits, so we didn't actually blow the assertion. We should NULL this out before invoking freemsg.

Actions
Copy link
 #1

Updated by Electric Monk 5 months ago

  • Gerrit CR set to 2591
Actions
Copy link
 #2

Updated by Luqman Aden 5 months ago

Tested with an adhoc misc module to allocb and create a chain before calling mac_drop_chain on it:

#include <sys/cmn_err.h>
#include <sys/mac_impl.h>
#include <sys/modctl.h>

#define MACTEST_INFO    "MAC testing module" 

static struct modlmisc i_mactest_modlmisc = {
    &mod_miscops,
    MACTEST_INFO
};

static struct modlinkage i_mactest_modlinkage = {
    MODREV_1,
    &i_mactest_modlmisc,
    NULL
};

int
_init(void)
{
    int err;
    mblk_t *bp1 = NULL, *bp2 = NULL;

    if ((err = mod_install(&i_mactest_modlinkage)) != 0) {
        return (err);
    }

    cmn_err(CE_NOTE, "%s loaded", MACTEST_INFO);

    if ((bp1 = allocb(1, BPRI_LO)) == NULL) {
        cmn_err(CE_WARN, "bp1 allocb failed");
        goto bail;
    }

    if ((bp2 = allocb(1, BPRI_LO)) == NULL) {
        cmn_err(CE_WARN, "bp2 allocb failed");
        goto bail;
    }

    // chain messages
    bp1->b_next = bp2;

    // free the chain
    mac_drop_chain(bp1, "mactest");

    cmn_err(CE_NOTE, "mactest: chain dropped");

    bp1 = NULL;
    bp2 = NULL;

    return (0);

bail:
    if (bp2 != NULL) {
        freeb(bp2);
    }
    if (bp1 != NULL) {
        freeb(bp1);
    }

    if (mod_remove(&i_mactest_modlinkage) != 0) {
        cmn_err(CE_WARN, "failed to unload during _init failure");
    }

    return (err);
}

int
_fini(void)
{
    int err;

    if ((err = mod_remove(&i_mactest_modlinkage)) != 0) {
        return (err);
    }

    cmn_err(CE_NOTE, "%s unloaded", MACTEST_INFO);

    return (0);
}

int
_info(struct modinfo *modinfop)
{
    return (mod_info(&i_mactest_modlinkage, modinfop));
}

With a DEBUG build before my changes it panics as expected:

panicstr = assertion failed: mp->b_next == NULL && mp->b_prev == NULL, file: ../../common/io/stream.c, line: 659
panicstack = fffffffffbdcb6e5 () | genunix:freemsg+5c () | mac:mac_drop_chain+9e () | mactest:_init+8a () | genunix:modinstall+105 () | genunix:mod_hold_installed_mod+77 () | genunix:modrload+1a6 () | genunix:modload+d () | genunix:mod_hold_dev_by_major+9f () | genunix:ddi_hold_driver+e () | devinfo:di_ioctl+32c () | genunix:cdev_ioctl+2b () | specfs:spec_ioctl+45 () | genunix:fop_ioctl+5b () | genunix:ioctl+153 () | unix:brand_sys_sysenter+2d2 ()

With the change things seem to work as expected:

Jan 11 19:53:40 luq-helios mactest: [ID 741872 kern.notice] NOTICE: MAC testing module loaded
Jan 11 19:53:40 luq-helios mactest: [ID 836213 kern.notice] NOTICE: mactest: chain dropped
Jan 11 19:53:54 luq-helios mactest: [ID 961466 kern.notice] NOTICE: MAC testing module unloaded

Actions
Copy link
 #3

Updated by Robert Mustacchi 5 months ago

  • Category set to networking
  • Assignee changed from Robert Mustacchi to Luqman Aden
Actions
Copy link
 #4

Updated by Electric Monk 5 months ago

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

git commit ff4179b7dc5b2168fbf912e04b1eff92bef994a3

commit  ff4179b7dc5b2168fbf912e04b1eff92bef994a3
Author: Luqman Aden <luqman@oxide.computer>
Date:   2023-01-12T19:43:09.000Z

    15305 mac_drop_chain calls freemsg with non-singleton chain
    Reviewed by: Marco van Wieringen <marco.van.wieringen@planets.elm.net>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Approved by: Dan McDonald <danmcd@mnx.io>

Actions
Copy link

Also available in: Atom PDF