Bug 219525
| Summary: | Multiple bugs in mpr ioctl handler | ||
|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> |
| Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> |
| Status: | New --- | ||
| Severity: | Affects Only Me | CC: | cem, emaste, op, slm |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Thanks for the report. I know there are a few issues there, especially with the driver not checking for bad user supplied values. I agree with most of your comments, but I have a question.
In this section:
.....
mpr_lock(sc);
size = data->Size;
if ((size >= sizeof(sc->recorded_events)) && (status == 0)) {
mpr_unlock(sc);
if (copyout((void *)sc->recorded_events,
PTRIN(data->PtrEvents), size) != 0)
status = EFAULT;
mpr_lock(sc);
} else {
/*
* data->Size value is not large enough to copy event data.
*/
status = EFAULT;
}
/*
* Change size value to match the number of bytes that were copied.
*/
if (status == 0)
data->Size = sizeof(sc->recorded_events);
mpr_unlock(sc);
.....
You say:
"It checks if the user supplied size is greater than the the size of the struct, and if so, copies using the size which it just determined was too large. It should `copyout` using `sizeof(sc->recorded_events)`."
But this is just checking if the user supplied size is enough to hold sizeof(sc->recored_events). It's not checking if it was too large, it's checking if it's large enough. If that user supplied size is >= to the data being copied, it should be OK. Do you agree?
I'm not sure I like that data->Size is being changed from the user supplied value, but that's something different. Maybe that's OK, but not sure. It could be that this is a normal technique used to return the number of bytes that are valid in the user buffer, but I only see that being done in this one function.
Thanks for taking a look.
To clarify the point you asked about:
`recorded_events` is a fixed size, it looks like this in `struct mpr_softc`:
mpr_event_entry_t recorded_events[MPR_EVENT_QUEUE_SIZE];
The kernel should copy out only `sizeof(sc->recored_events)` bytes at most from here, or else it will copy out of bounds memory to userland.
Going back to the code:
size = data->Size;
if ((size >= sizeof(sc->recorded_events)) && (status == 0)) {
mpr_unlock(sc);
if (copyout((void *)sc->recorded_events,
PTRIN(data->PtrEvents), size) != 0)
status = EFAULT;
mpr_lock(sc);
}
Notice that it passes `size` to `copyout`, which can be greater than `sizeof(sc->recorded_events)` (see the check).
Instead, the `copyout` call should be passed `sizeof(sc->recorded_events)` so that it won't read more than the array contains.
So, you are correct when you say that it "is just checking if the user supplied size is enough to hold sizeof(sc->recored_events)". But this check won't prevent copying out of bounds memory to userland.
OK. I see what you're saying. The check makes sure that data is not copied to invalid space, but it does not check if the bytes are valid. That's true.
Maybe it's better like this:
if (status == 0) {
if (copyout((void *)sc->recorded_events, PTRIN(data->PtrEvents),
min(size, sizeof(sc->recorded_events))) != 0)
status = EFAULT;
}
Then, it just copies out as many valid bytes as it can, and no 'else' part.
And there's no reason to look at 'status' in the 'if' actually, so just:
if (copyout((void *)sc->recorded_events, PTRIN(data->PtrEvents),
min(size, sizeof(sc->recorded_events))) != 0)
status = EFAULT;
That's correct, using the `min` macro will correct that issue. |
There are multiple bugs in the `mpr` device's ioctl handler, `mpr_ioctl`. Multiple `malloc` calls are made with unlimited, user controlled, unsigned 32bit integers and the `M_WAITOK` flag, meaning that if a large enough request is made, it can crash the system. `M_WAITOK` should not be passed, and the call should return `ENOMEM` if `malloc` failed (along with any necessary cleanup). case MPRIO_READ_CFG_PAGE: mpr_page = malloc(page_req->len, M_MPRUSER, M_WAITOK | M_ZERO); ... case MPRIO_READ_EXT_CFG_PAGE: mpr_page = malloc(ext_page_req->len, M_MPRUSER, M_WAITOK | M_ZERO); ... case MPRIO_WRITE_CFG_PAGE: mpr_page = malloc(page_req->len, M_MPRUSER, M_WAITOK|M_ZERO); The `MPTIOCTL_PASS_THRU` command leads to a `copyin` call with user supplied, unchecked, unsigned 32bit integer, and destination a struct on the stack. Leading to stack overflow with arbitrary contents and size. The copy size here should be `sizeof(tmphdr)` instead. This command then performs some validation on `data->RequestSize`, but at this point `mpr_lock` is not held, and so `IOCRequestFrameSize` value can change, and this check can be raced, if for example a Diag Reset is made in another thread (ioctl command `MPTIOCTL_RESET_ADAPTER` can do this for example). This same bug is repeated in ioctl command `MPRIO_MPR_COMMAND`. A `copyout` call can be made with user supplied unsigned 32bit integer `ReplySize`. The code checks if `sz > data->ReplySize`, but doesn't check if `sz < data->ReplySize`. Results in copying out of bounds memory to userland. The header copied in from userland earlier is then copied to task, and uses `data->RequestSize`, which should just be the size of the source struct, `sizeof(tmphdr)`. This happens again later on as well. Another `malloc` call is made with unlimited, user controlled, unsigned 32bit integer and `M_WAITOK` flag. case MPTIOCTL_PASS_THRU: /* * The user has requested to pass through a command to be * executed by the MPT firmware. Call our routine which does * this. Only allow one passthru IOCTL at one time. */ error = mpr_user_pass_thru(sc, (mpr_pass_thru_t *)arg); break; static int mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) { MPI2_REQUEST_HEADER *hdr, tmphdr; ... /* * copy in the header so we know what we're dealing with before we * commit to allocating a command for it. */ err = copyin(PTRIN(data->PtrRequest), &tmphdr, data->RequestSize); if (err != 0) goto RetFreeUnlocked; if (data->RequestSize > (int)sc->facts->IOCRequestFrameSize * 4) { err = EINVAL; goto RetFreeUnlocked; ... if (sz > data->ReplySize) { mpr_printf(sc, "%s: user reply buffer (%d) " "smaller than returned buffer (%d)\n", __func__, data->ReplySize, sz); } mpr_unlock(sc); copyout(cm->cm_reply, PTRIN(data->PtrReply), data->ReplySize); mpr_lock(sc); ... /* Copy the header in. Only a small fixup is needed. */ task = (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req; bcopy(&tmphdr, task, data->RequestSize); ... hdr = (MPI2_REQUEST_HEADER *)cm->cm_req; bcopy(&tmphdr, hdr, data->RequestSize); ... cm->cm_length = MAX(data->DataSize, data->DataOutSize); cm->cm_out_len = data->DataOutSize; cm->cm_flags = 0; if (cm->cm_length != 0) { cm->cm_data = malloc(cm->cm_length, M_MPRUSER, M_WAITOK | M_ZERO); ... } The `MPTIOCTL_EVENT_REPORT` command calls `mpr_user_event_report`, which incorrectly checks user supplied unsigned 32bit size. It checks if the user supplied size is greater than the the size of the struct, and if so, copies using the size which it just determined was too large. It should `copyout` using `sizeof(sc->recorded_events)`. case MPTIOCTL_EVENT_REPORT: /* * The user has done an event report. Call our routine which * does this. */ error = mpr_user_event_report(sc, (mpr_event_report_t *)arg); break; static int mpr_user_event_report(struct mpr_softc *sc, mpr_event_report_t *data) { int status = 0; uint32_t size; mpr_lock(sc); size = data->Size; if ((size >= sizeof(sc->recorded_events)) && (status == 0)) { mpr_unlock(sc); if (copyout((void *)sc->recorded_events, PTRIN(data->PtrEvents), size) != 0) status = EFAULT; mpr_lock(sc); } else { /* * data->Size value is not large enough to copy event data. */ status = EFAULT; } /* * Change size value to match the number of bytes that were copied. */ if (status == 0) data->Size = sizeof(sc->recorded_events); mpr_unlock(sc); return (status); } The `MPTIOCTL_REG_ACCESS` command doesn't validate user supplied, unsigned 32bit `RegOffset` integer. This command leads to `bus_space_write_4` with arbitrary offset. From the man page: "The bus_space_write_N() family of functions writes a 1, 2, 4, or 8 byte data item to the offset specified by offset into the region specified by handle of the bus space specified by space. The location being written must lie within the bus space region specified by handle." case MPTIOCTL_REG_ACCESS: /* * The user has requested register access. Call our routine * which does this. */ mpr_lock(sc); error = mpr_user_reg_access(sc, (mpr_reg_access_t *)arg); mpr_unlock(sc); break; static int mpr_user_reg_access(struct mpr_softc *sc, mpr_reg_access_t *data) { int status = 0; switch (data->Command) { ... case REG_MEM_READ: data->RegData = mpr_regread(sc, data->RegOffset); break; case REG_MEM_WRITE: mpr_regwrite(sc, data->RegOffset, data->RegData); break; ... } return (status); } ... static __inline void mpr_regwrite(struct mpr_softc *sc, uint32_t offset, uint32_t val) { bus_space_write_4(sc->mpr_btag, sc->mpr_bhandle, offset, val); } The `MPTIOCTL_BTDH_MAPPING` command leads to an off-by-one error in `mpr_user_btdh`. case MPTIOCTL_BTDH_MAPPING: /* * The user has requested to translate a bus/target to a * DevHandle or a DevHandle to a bus/target. Call our routine * which does this. */ error = mpr_user_btdh(sc, (mpr_btdh_mapping_t *)arg); break; static int mpr_user_btdh(struct mpr_softc *sc, mpr_btdh_mapping_t *data) { uint8_t bt2dh = FALSE; uint8_t dh2bt = FALSE; uint16_t dev_handle, bus, target; bus = data->Bus; target = data->TargetID; dev_handle = data->DevHandle; ... if (target > sc->max_devices) { mpr_dprint(sc, MPR_XINFO, "Target ID is out of range " "for Bus/Target to DevHandle mapping."); return (EINVAL); } dev_handle = sc->mapping_table[target].dev_handle; ... return (0); } The check for `target` is that an error should be returned if `target > sc->max_devices`, however this check should be changed to `target >= sc->max_devices`, since if `target` is equal to `max_devices` it will be an out of bounds access, since `mapping_table` is allocated with `max_devices` elements: int mpr_mapping_allocate_memory(struct mpr_softc *sc) { uint32_t dpm_pg0_sz; sc->mapping_table = malloc((sizeof(struct dev_mapping_table) * sc->max_devices), M_MPR, M_ZERO|M_NOWAIT); ...