https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219525
Bug ID: 219525
Summary: Multiple bugs in mpr ioctl handler
Product: Base System
Version: CURRENT
Hardware: Any
OS: Any
Status: New
Severity: Affects Only Me
Priority: ---
Component: kern
Assignee: freebsd-bugs@FreeBSD.org
Reporter: ect...@gmail.com
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);
...
--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
freebsd-bugs@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
