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));
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.
Thanks for your submission CTurt. Please feel to to attach a proposed change to resolve
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));
Any movement on this?
Created attachment 169497 [details] Check return values from copyin and copyout
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.
batch change of PRs untouched in 2018 marked "in progress" back to open.
did a check of stable releases as well. This code doesn't appear to exist any more.