Bug 206581 - bxe_ioctl_nvram handler is faulty
Summary: bxe_ioctl_nvram handler is faulty
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-net
URL:
Keywords: needs-qa, patch, security
Depends on:
Blocks:
 
Reported: 2016-01-24 16:31 UTC by CTurt
Modified: 2016-04-26 15:59 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable9?
koobs: mfc-stable10?


Attachments
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 -
           sizeof(uint32_t));

    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 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.

`bxe_ioctl_nvram`:

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

    ...

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

`bxe_ioctl`:

    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.