diff options
author | Mark Johnston <markj@FreeBSD.org> | 2021-01-08 18:32:04 +0000 |
---|---|---|
committer | Mark Johnston <markj@FreeBSD.org> | 2021-01-08 18:32:04 +0000 |
commit | de828a91db29fb20440e0d92f3d3136b314a9584 (patch) | |
tree | 5973d7f5904c606477f635994d09ab810e2f83ac | |
parent | 092cf8d63f9fb9153c677f5793135926edc101d7 (diff) | |
download | src-de828a91db29fb20440e0d92f3d3136b314a9584.tar.gz src-de828a91db29fb20440e0d92f3d3136b314a9584.zip |
mpr, mps: Fix a stack buffer overflow in the user passthru ioctl
Previously we copied in the request into a stack-allocated structure
that could be smaller than the request size. Furthermore, we checked
the request size only after doing the copyin.
Fix this by allocating a buffer to hold the request, then copying the
buffer's contents into a command descriptor. This is a bit heavy-handed
but I expect the overhead will not be noticeable. The approach of
coping the header in first is susceptible to TOCTOU problems.
Reviewed by: imp
Reported by: maxpl0it@protonmail.com
MFC after: 3 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D27963
-rw-r--r-- | sys/dev/mpr/mpr_user.c | 30 | ||||
-rw-r--r-- | sys/dev/mps/mps_user.c | 32 |
2 files changed, 31 insertions, 31 deletions
diff --git a/sys/dev/mpr/mpr_user.c b/sys/dev/mpr/mpr_user.c index c0d35237d2f7..236e5c9715df 100644 --- a/sys/dev/mpr/mpr_user.c +++ b/sys/dev/mpr/mpr_user.c @@ -737,11 +737,12 @@ RetFree: static int mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) { - MPI2_REQUEST_HEADER *hdr, tmphdr; + MPI2_REQUEST_HEADER *hdr, *tmphdr; MPI2_DEFAULT_REPLY *rpl; Mpi26NVMeEncapsulatedErrorReply_t *nvme_error_reply = NULL; Mpi26NVMeEncapsulatedRequest_t *nvme_encap_request = NULL; struct mpr_command *cm = NULL; + void *req = NULL; int i, err = 0, dir = 0, sz; uint8_t tool, function = 0; u_int sense_len; @@ -793,22 +794,21 @@ mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) data->ReplySize, data->PtrData, data->DataSize, data->PtrDataOut, data->DataOutSize, data->DataDirection); - /* - * 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->reqframesz) { + if (data->RequestSize > sc->reqframesz) { err = EINVAL; goto RetFreeUnlocked; } - function = tmphdr.Function; + req = malloc(data->RequestSize, M_MPRUSER, M_WAITOK | M_ZERO); + tmphdr = (MPI2_REQUEST_HEADER *)req; + + err = copyin(PTRIN(data->PtrRequest), req, data->RequestSize); + if (err != 0) + goto RetFreeUnlocked; + + function = tmphdr->Function; mpr_dprint(sc, MPR_USER, "%s: Function %02X MsgFlags %02X\n", __func__, - function, tmphdr.MsgFlags); + function, tmphdr->MsgFlags); /* * Handle a passthru TM request. @@ -825,7 +825,7 @@ mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) /* Copy the header in. Only a small fixup is needed. */ task = (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req; - bcopy(&tmphdr, task, data->RequestSize); + memcpy(task, req, data->RequestSize); task->TaskMID = cm->cm_desc.Default.SMID; cm->cm_data = NULL; @@ -872,7 +872,6 @@ mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) mpr_lock(sc); cm = mpr_alloc_command(sc); - if (cm == NULL) { mpr_printf(sc, "%s: no mpr requests\n", __func__); err = ENOMEM; @@ -881,7 +880,7 @@ mpr_user_pass_thru(struct mpr_softc *sc, mpr_pass_thru_t *data) mpr_unlock(sc); hdr = (MPI2_REQUEST_HEADER *)cm->cm_req; - bcopy(&tmphdr, hdr, data->RequestSize); + memcpy(hdr, req, data->RequestSize); /* * Do some checking to make sure the IOCTL request contains a valid @@ -1154,6 +1153,7 @@ RetFree: Ret: sc->mpr_flags &= ~MPR_FLAGS_BUSY; mpr_unlock(sc); + free(req, M_MPRUSER); return (err); } diff --git a/sys/dev/mps/mps_user.c b/sys/dev/mps/mps_user.c index 650e5d5e3e87..c80fbc68cdf3 100644 --- a/sys/dev/mps/mps_user.c +++ b/sys/dev/mps/mps_user.c @@ -677,7 +677,7 @@ mps_user_command(struct mps_softc *sc, struct mps_usr_command *cmd) mps_dprint(sc, MPS_USER, "%s: req %p %d rpl %p %d\n", __func__, cmd->req, cmd->req_len, cmd->rpl, cmd->rpl_len); - if (cmd->req_len > (int)sc->reqframesz) { + if (cmd->req_len > sc->reqframesz) { err = EINVAL; goto RetFreeUnlocked; } @@ -750,9 +750,10 @@ RetFree: static int mps_user_pass_thru(struct mps_softc *sc, mps_pass_thru_t *data) { - MPI2_REQUEST_HEADER *hdr, tmphdr; + MPI2_REQUEST_HEADER *hdr, *tmphdr; MPI2_DEFAULT_REPLY *rpl = NULL; struct mps_command *cm = NULL; + void *req = NULL; int err = 0, dir = 0, sz; uint8_t function = 0; u_int sense_len; @@ -804,22 +805,21 @@ mps_user_pass_thru(struct mps_softc *sc, mps_pass_thru_t *data) data->ReplySize, data->PtrData, data->DataSize, data->PtrDataOut, data->DataOutSize, data->DataDirection); - /* - * 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->reqframesz) { + if (data->RequestSize > sc->reqframesz) { err = EINVAL; goto RetFreeUnlocked; } - function = tmphdr.Function; + req = malloc(data->RequestSize, M_MPSUSER, M_WAITOK | M_ZERO); + tmphdr = (MPI2_REQUEST_HEADER *)req; + + err = copyin(PTRIN(data->PtrRequest), req, data->RequestSize); + if (err != 0) + goto RetFreeUnlocked; + + function = tmphdr->Function; mps_dprint(sc, MPS_USER, "%s: Function %02X MsgFlags %02X\n", __func__, - function, tmphdr.MsgFlags); + function, tmphdr->MsgFlags); /* * Handle a passthru TM request. @@ -836,7 +836,7 @@ mps_user_pass_thru(struct mps_softc *sc, mps_pass_thru_t *data) /* Copy the header in. Only a small fixup is needed. */ task = (MPI2_SCSI_TASK_MANAGE_REQUEST *)cm->cm_req; - bcopy(&tmphdr, task, data->RequestSize); + memcpy(task, req, data->RequestSize); task->TaskMID = cm->cm_desc.Default.SMID; cm->cm_data = NULL; @@ -883,7 +883,6 @@ mps_user_pass_thru(struct mps_softc *sc, mps_pass_thru_t *data) mps_lock(sc); cm = mps_alloc_command(sc); - if (cm == NULL) { mps_printf(sc, "%s: no mps requests\n", __func__); err = ENOMEM; @@ -892,7 +891,7 @@ mps_user_pass_thru(struct mps_softc *sc, mps_pass_thru_t *data) mps_unlock(sc); hdr = (MPI2_REQUEST_HEADER *)cm->cm_req; - bcopy(&tmphdr, hdr, data->RequestSize); + memcpy(hdr, req, data->RequestSize); /* * Do some checking to make sure the IOCTL request contains a valid @@ -1059,6 +1058,7 @@ RetFreeUnlocked: Ret: sc->mps_flags &= ~MPS_FLAGS_BUSY; mps_unlock(sc); + free(req, M_MPSUSER); return (err); } |