Bug 206581
| Summary: | bxe_ioctl_nvram handler is faulty | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> | ||||
| Component: | kern | Assignee: | freebsd-net (Nobody) <net> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | op, sbruno, secteam, shawn.webb | ||||
| Priority: | Normal | Keywords: | needs-qa, patch, security | ||||
| Version: | CURRENT | Flags: | sbruno:
mfc-stable10+
|
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
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. |
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));