Date: Mon, 11 Jan 2021 14:54:02 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: ed6fa9d618bf - stable/12 - mpr, mps: Fix a stack buffer overflow in the user passthru ioctl Message-ID: <202101111454.10BEs2VB073777@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/12 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=ed6fa9d618bff47dcd3fb000e5805e29d331578d commit ed6fa9d618bff47dcd3fb000e5805e29d331578d Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2021-01-08 18:32:04 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2021-01-11 14:43:37 +0000 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 Differential Revision: https://reviews.freebsd.org/D27963 (cherry picked from commit de828a91db29fb20440e0d92f3d3136b314a9584) --- sys/dev/mpr/mpr_user.c | 30 +++++++++++++++--------------- 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 1b2aa398b22b..3995d01154f7 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 af1526c36d4c..ab4d1d2d86f3 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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202101111454.10BEs2VB073777>