Bug 206581 - bxe_ioctl_nvram handler is faulty
Summary: bxe_ioctl_nvram handler is faulty
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-net mailing list
Keywords: needs-qa, patch, security
Depends on:
Reported: 2016-01-24 16:31 UTC by CTurt
Modified: 2018-05-23 14:13 UTC (History)
4 users (show)

See Also:
sbruno: mfc-stable10+

Check return values from copyin and copyout (4.43 KB, patch)
2016-04-20 16:32 UTC, CTurt
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-24 16:31:43 UTC
Take a look at the start of `bxe_ioctl_nvram` from `sys/dev/bxe/bxe.c`:

static int
bxe_ioctl_nvram(struct bxe_softc *sc,
                uint32_t         priv_op,
                struct ifreq     *ifr)
    struct bxe_nvram_data nvdata_base;
    struct bxe_nvram_data *nvdata;
    int len;
    int error = 0;

    copyin(ifr->ifr_data, &nvdata_base, sizeof(nvdata_base));

    len = (sizeof(struct bxe_nvram_data) +
           nvdata_base.len -

    if (len > sizeof(struct bxe_nvram_data)) {
        if ((nvdata = (struct bxe_nvram_data *)
                 malloc(len, M_DEVBUF,
                        (M_NOWAIT | M_ZERO))) == NULL) {
            BLOGE(sc, "BXE_IOC_RD_NVRAM malloc failed\n");
            return (1);
        memcpy(nvdata, &nvdata_base, sizeof(struct bxe_nvram_data));
    } else {
        nvdata = &nvdata_base;

Firstly, the result from `copyin` isn't even checked here...

Secondly, no bound checks on user supplied `nvdata_base.len`, means we can get `len` to overflow (since it is a `signed int`). For example, give an input length of `0x80000000 + sizeof(uint32_t)) - (sizeof(struct bxe_nvram_data)` to get an allocation of 0 bytes, and then boom, we have heap overflow straight after:

    memcpy(nvdata, &nvdata_base, sizeof(struct bxe_nvram_data));
Comment 1 CTurt 2016-01-24 16:37:28 UTC
Sorry, forgot about the check:

    if (len > sizeof(struct bxe_nvram_data)) {

So, the example I suggested wouldn't work.

But the lack of `copyin` being checked, is still valid. And there probably should be some bound checks anyway.
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2016-01-24 16:53:55 UTC
Thanks for your submission CTurt.

Please feel to to attach a proposed change to resolve
Comment 3 CTurt 2016-01-29 22:39:31 UTC
To clarify my original post, the bound check is fine.

However, there is a problem that multiple parts of this code use `copyin` without checking the result, which could possibly lead to the use of uninitialised stack data if the `copyin` calls fail.


    copyin(ifr->ifr_data, &nvdata_base, sizeof(nvdata_base));


        copyin(ifr->ifr_data, nvdata, len);
        error = bxe_nvram_write(sc,
                                (uint8_t *)nvdata->value,


    copyin(ifr->ifr_data, &priv_op, sizeof(priv_op));
Comment 4 Shawn Webb 2016-03-01 20:57:48 UTC
Any movement on this?
Comment 5 CTurt 2016-04-20 16:32:21 UTC
Created attachment 169497 [details]
Check return values from copyin and copyout
Comment 6 Sean Bruno freebsd_committer 2016-04-26 15:59:08 UTC
Maintainer deleted this code in revision https://svnweb.freebsd.org/base?view=revision&revision=298496

This ticket can be closed when the MFC period to 10 expires.
Comment 7 Eitan Adler freebsd_committer freebsd_triage 2018-05-23 10:27:33 UTC
batch change of PRs untouched in 2018 marked "in progress" back to open.
Comment 8 Sean Bruno freebsd_committer 2018-05-23 14:13:52 UTC
did a check of stable releases as well.  This code doesn't appear to exist any more.