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)

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);
 }