Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 14 May 2020 17:56:44 +0000 (UTC)
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org
Subject:   svn commit: r361038 - in stable: 11/sys/cam/scsi 11/sys/dev/iscsi_initiator 11/sys/net80211 12/sys/cam/scsi 12/sys/dev/iscsi_initiator 12/sys/net80211
Message-ID:  <202005141756.04EHuiMe085309@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jhb
Date: Thu May 14 17:56:43 2020
New Revision: 361038
URL: https://svnweb.freebsd.org/changeset/base/361038

Log:
  MFC 360171,360179,360285,360388: Don't dereference various user pointers.
  
  360171:
  Don't access a user buffer directly from the kernel.
  
  The handle_string callback for the ENCIOC_SETSTRING ioctl was passing
  a user pointer to memcpy().  Fix by using copyin() instead.
  
  For ENCIOC_GETSTRING ioctls, the handler was storing the user pointer
  in a CCB's data_ptr field where it was indirected by other code.  Fix
  this by allocating a temporary buffer (which ENCIOC_SETSTRING already
  did) and copying the result out to the user buffer after the CCB has
  been processed.
  
  360179:
  Don't pass a user buffer pointer as the data pointer in a CCB.
  
  Allocate a temporary buffer in the kernel to serve as the CCB data
  pointer for a pass-through transaction and use copyin/copyout to
  shuffle the data to/from the user buffer.
  
  360285:
  Don't indirect user pointers directly in two 802.11s ioctls.
  
  IEEE80211_MESH_RTCMD_ADD was invoking memcmp() to validate the
  supplied address directly on the user pointer rather than first doing
  a copyin() and validating the copied value.
  
  IEEE80211_MESH_RTCMD_DELETE was passing the user pointer directly to
  ieee80211_mesh_rt_del() rather than copying the user buffer into a
  temporary kernel buffer.
  
  360388:
  Don't run strcmp() against strings stored in user memory.
  
  Instead, copy the strings into a temporary buffer on the stack and
  run strcmp on the copies.

Modified:
  stable/12/sys/cam/scsi/scsi_enc_ses.c
  stable/12/sys/cam/scsi/scsi_sg.c
  stable/12/sys/dev/iscsi_initiator/isc_subr.c
  stable/12/sys/net80211/ieee80211_mesh.c
Directory Properties:
  stable/12/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/11/sys/cam/scsi/scsi_enc_ses.c
  stable/11/sys/cam/scsi/scsi_sg.c
  stable/11/sys/dev/iscsi_initiator/isc_subr.c
  stable/11/sys/net80211/ieee80211_mesh.c
Directory Properties:
  stable/11/   (props changed)

Modified: stable/12/sys/cam/scsi/scsi_enc_ses.c
==============================================================================
--- stable/12/sys/cam/scsi/scsi_enc_ses.c	Thu May 14 17:54:08 2020	(r361037)
+++ stable/12/sys/cam/scsi/scsi_enc_ses.c	Thu May 14 17:56:43 2020	(r361038)
@@ -2904,13 +2904,19 @@ ses_handle_string(enc_softc_t *enc, encioc_string_t *s
 		buf[1] = 0;
 		buf[2] = sstr->bufsiz >> 8;
 		buf[3] = sstr->bufsiz & 0xff;
-		memcpy(&buf[4], sstr->buf, sstr->bufsiz);
+		ret = copyin(sstr->buf, &buf[4], sstr->bufsiz);
+		if (ret != 0) {
+			ENC_FREE(buf);
+			return (ret);
+		}
 		break;
 	case ENCIOC_GETSTRING:
 		payload = sstr->bufsiz;
 		amt = payload;
+		buf = ENC_MALLOC(payload);
+		if (buf == NULL)
+			return (ENOMEM);
 		ses_page_cdb(cdb, payload, SesStringIn, CAM_DIR_IN);
-		buf = sstr->buf;
 		break;
 	case ENCIOC_GETENCNAME:
 		if (ses_cache->ses_nsubencs < 1)
@@ -2950,6 +2956,8 @@ ses_handle_string(enc_softc_t *enc, encioc_string_t *s
 		return (EINVAL);
 	}
 	ret = enc_runcmd(enc, cdb, 6, buf, &amt);
+	if (ret == 0 && ioc == ENCIOC_GETSTRING)
+		ret = copyout(buf, sstr->buf, sstr->bufsiz);
 	if (ioc == ENCIOC_SETSTRING)
 		ENC_FREE(buf);
 	return (ret);

Modified: stable/12/sys/cam/scsi/scsi_sg.c
==============================================================================
--- stable/12/sys/cam/scsi/scsi_sg.c	Thu May 14 17:54:08 2020	(r361037)
+++ stable/12/sys/cam/scsi/scsi_sg.c	Thu May 14 17:56:43 2020	(r361038)
@@ -508,6 +508,7 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 	struct cam_periph *periph;
 	struct sg_softc *softc;
 	struct sg_io_hdr *req;
+	void *data_ptr;
 	int dir, error;
 
 	periph = (struct cam_periph *)dev->si_drv1;
@@ -552,12 +553,20 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 			break;
 		}
 
+		if (req->dxfer_len > MAXPHYS) {
+			error = EINVAL;
+			break;
+		}
+
+		data_ptr = malloc(req->dxfer_len, M_DEVBUF, M_WAITOK);
+
 		ccb = cam_periph_getccb(periph, CAM_PRIORITY_NORMAL);
 		csio = &ccb->csio;
 
 		error = copyin(req->cmdp, &csio->cdb_io.cdb_bytes,
 		    req->cmd_len);
 		if (error) {
+			free(data_ptr, M_DEVBUF);
 			xpt_release_ccb(ccb);
 			break;
 		}
@@ -570,7 +579,7 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 			dir = CAM_DIR_IN;
 			break;
 		case SG_DXFER_TO_FROM_DEV:
-			dir = CAM_DIR_IN | CAM_DIR_OUT;
+			dir = CAM_DIR_BOTH;
 			break;
 		case SG_DXFER_NONE:
 		default:
@@ -578,12 +587,21 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 			break;
 		}
 
+		if (dir == CAM_DIR_IN || dir == CAM_DIR_BOTH) {
+			error = copyin(req->dxferp, data_ptr, req->dxfer_len);
+			if (error) {
+				free(data_ptr, M_DEVBUF);
+				xpt_release_ccb(ccb);
+				break;
+			}
+		}
+
 		cam_fill_csio(csio,
 			      /*retries*/1,
 			      /*cbfcnp*/NULL,
 			      dir|CAM_DEV_QFRZDIS,
 			      MSG_SIMPLE_Q_TAG,
-			      req->dxferp,
+			      data_ptr,
 			      req->dxfer_len,
 			      req->mx_sb_len,
 			      req->cmd_len,
@@ -593,6 +611,7 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 		if (error) {
 			req->host_status = DID_ERROR;
 			req->driver_status = DRIVER_INVALID;
+			free(data_ptr, M_DEVBUF);
 			xpt_release_ccb(ccb);
 			break;
 		}
@@ -611,6 +630,10 @@ sgioctl(struct cdev *dev, u_long cmd, caddr_t arg, int
 					req->sb_len_wr);
 		}
 
+		if ((dir == CAM_DIR_OUT || dir == CAM_DIR_BOTH) && error == 0)
+			error = copyout(data_ptr, req->dxferp, req->dxfer_len);
+
+		free(data_ptr, M_DEVBUF);
 		xpt_release_ccb(ccb);
 		break;
 		

Modified: stable/12/sys/dev/iscsi_initiator/isc_subr.c
==============================================================================
--- stable/12/sys/dev/iscsi_initiator/isc_subr.c	Thu May 14 17:54:08 2020	(r361037)
+++ stable/12/sys/dev/iscsi_initiator/isc_subr.c	Thu May 14 17:56:43 2020	(r361038)
@@ -194,6 +194,9 @@ i_crc32c(const void *buf, size_t size, uint32_t crc)
 int
 i_setopt(isc_session_t *sp, isc_opt_t *opt)
 {
+     char buf[16];
+     int error;
+
      if(opt->maxRecvDataSegmentLength > 0) {
 	  sp->opt.maxRecvDataSegmentLength = opt->maxRecvDataSegmentLength;
 	  sdebug(2, "maxRecvDataSegmentLength=%d", sp->opt.maxRecvDataSegmentLength);
@@ -235,15 +238,21 @@ i_setopt(isc_session_t *sp, isc_opt_t *opt)
      }
 
      if(opt->headerDigest != NULL) {
-	  sdebug(2, "opt.headerDigest='%s'", opt->headerDigest);
-	  if(strcmp(opt->headerDigest, "CRC32C") == 0) {
+	  error = copyinstr(opt->headerDigest, buf, sizeof(buf), NULL);
+	  if (error != 0)
+	       return (error);
+	  sdebug(2, "opt.headerDigest='%s'", buf);
+	  if(strcmp(buf, "CRC32C") == 0) {
 	       sp->hdrDigest = (digest_t *)i_crc32c;
 	       sdebug(2, "opt.headerDigest set");
 	  }
      }
      if(opt->dataDigest != NULL) {
-	  sdebug(2, "opt.dataDigest='%s'", opt->headerDigest);
-	  if(strcmp(opt->dataDigest, "CRC32C") == 0) {
+	  error = copyinstr(opt->dataDigest, buf, sizeof(buf), NULL);
+	  if (error != 0)
+	       return (error);
+	  sdebug(2, "opt.dataDigest='%s'", opt->dataDigest);
+	  if(strcmp(buf, "CRC32C") == 0) {
 	       sp->dataDigest = (digest_t *)i_crc32c;
 	       sdebug(2, "opt.dataDigest set");
 	  }

Modified: stable/12/sys/net80211/ieee80211_mesh.c
==============================================================================
--- stable/12/sys/net80211/ieee80211_mesh.c	Thu May 14 17:54:08 2020	(r361037)
+++ stable/12/sys/net80211/ieee80211_mesh.c	Thu May 14 17:56:43 2020	(r361038)
@@ -3569,16 +3569,21 @@ mesh_ioctl_set80211(struct ieee80211vap *vap, struct i
 			ieee80211_mesh_rt_flush(vap);
 			break;
 		case IEEE80211_MESH_RTCMD_ADD:
-			if (IEEE80211_ADDR_EQ(vap->iv_myaddr, ireq->i_data) ||
-			    IEEE80211_ADDR_EQ(broadcastaddr, ireq->i_data))
-				return EINVAL;
-			error = copyin(ireq->i_data, &tmpaddr,
+			error = copyin(ireq->i_data, tmpaddr,
 			    IEEE80211_ADDR_LEN);
-			if (error == 0)
-				ieee80211_mesh_discover(vap, tmpaddr, NULL);
+			if (error != 0)
+				break;
+			if (IEEE80211_ADDR_EQ(vap->iv_myaddr, tmpaddr) ||
+			    IEEE80211_ADDR_EQ(broadcastaddr, tmpaddr))
+				return EINVAL;
+			ieee80211_mesh_discover(vap, tmpaddr, NULL);
 			break;
 		case IEEE80211_MESH_RTCMD_DELETE:
-			ieee80211_mesh_rt_del(vap, ireq->i_data);
+			error = copyin(ireq->i_data, tmpaddr,
+			    IEEE80211_ADDR_LEN);
+			if (error != 0)
+				break;
+			ieee80211_mesh_rt_del(vap, tmpaddr);
 			break;
 		default:
 			return ENOSYS;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202005141756.04EHuiMe085309>