ocs_fc: Harden ioctl, XRI lookup and FCP command validation

Fix several issues found by security review of the driver.

ioctl:
- Require PRIV_DRIVER for all ocs ioctls and create the device node
  with mode 0600, group wheel.
- Return the error from ocs_process_sli_config() to the caller.
  Previously only ENXIO was propagated, so a failed validation or a
  failed copyin() still issued the mailbox command, in the latter case
  with a descriptor pointing at freed memory.
- Reject COMMON_WRITE_OBJECT when in_bytes exceeds
  desired_write_length, and cap WRITE_OBJECT, READ_OBJECT,
  READ_OBJECT_LIST and non-embedded mailbox buffers at
  OCS_MAX_DMA_ALLOC, returning EINVAL.
- Use a 64-bit size for the non-embedded mailbox buffer to avoid
  truncation of a user supplied length.
- Refuse to copy out more than the size of the DMA buffer allocated
  for the command.
- Range check the BAR offset in the ECD helper ioctl.

HW:
- Range check the XRI in ocs_hw_io_lookup() and return NULL when it is
  out of range.  Handle a NULL return in the auto xfer ready command
  and data paths (returning the RQ buffers in the former), and in
  ocs_hw_reclaim_xri().
- Make ocs_ref_get_unless_zero() atomic.

FCP:
- additional_fcp_cdb_length is in 4-byte words, but
  ocs_validate_fcp_cmd() added it to the expected payload length as
  bytes, so the length check under-validated the received frame.
- Drop commands with a CDB longer than CAM_MAX_CDBLEN instead of
  passing a truncated CDB to CAM.

MFC after:	1 week
Sponsored by:	Broadcom
Reviewed by:	ken

sys/dev/ocs_fc/ocs_cam.c

@@ -607,6 +607,8 @@ int32_t ocs_scsi_recv_cmd(ocs_io_t *io, uint64_t lun, uint8_t *cdb,
 		atio->priority = (flags & OCS_SCSI_PRIORITY_MASK) >>
 		    OCS_SCSI_PRIORITY_SHIFT;
 
+		if (cdb_len > sizeof(atio->cdb_io.cdb_bytes))
+			cdb_len = sizeof(atio->cdb_io.cdb_bytes);
 		atio->cdb_len = cdb_len;
 		ocs_memcpy(atio->cdb_io.cdb_bytes, cdb, cdb_len);
 

sys/dev/ocs_fc/ocs_hw.c

@@ -9736,7 +9736,14 @@ ocs_hw_io_t *
 ocs_hw_io_lookup(ocs_hw_t *hw, uint32_t xri)
 {
 	uint32_t ioindex;
-	ioindex = xri - hw->sli.config.extent[SLI_RSRC_FCOE_XRI].base[0];
+	uint32_t base = hw->sli.config.extent[SLI_RSRC_FCOE_XRI].base[0];
+
+	if (xri < base || (xri - base) >= hw->config.n_io) {
+		ocs_log_err(hw->os, "xri %#x out of range (base=%#x n_io=%u)\n",
+		    xri, base, hw->config.n_io);
+		return NULL;
+	}
+	ioindex = xri - base;
 	return hw->io[ioindex];
 }
 
@@ -11346,6 +11353,8 @@ ocs_hw_reclaim_xri(ocs_hw_t *hw, uint16_t xri_base, uint16_t xri_count)
 
 	for (i = 0; i < xri_count; i++) {
 		io = ocs_hw_io_lookup(hw, xri_base + i);
+		if (io == NULL)
+			continue;
 
 		/*
 		 * if this is an auto xfer rdy XRI, then we need to release any

sys/dev/ocs_fc/ocs_hw_queues.c

@@ -1873,6 +1873,12 @@ ocs_hw_rqpair_process_auto_xfr_rdy_cmd(ocs_hw_t *hw, hw_cq_t *cq, uint8_t *cqe)
 		fc_header_t *fc_hdr = seq->header->dma.virt;
 
 		seq->hio = ocs_hw_io_lookup(hw, seq->xri);
+		if (seq->hio == NULL) {
+			ocs_log_err(hw->os, "auto_xrdy CQE, invalid xri=%#x\n",
+			    seq->xri);
+			ocs_hw_rqpair_sequence_free(hw, seq);
+			return -1;
+		}
 		ocs_lock(&seq->hio->axr_lock);
 		axr_lock_taken = 1;
 
@@ -1971,6 +1977,11 @@ ocs_hw_rqpair_process_auto_xfr_rdy_data(ocs_hw_t *hw, hw_cq_t *cq, uint8_t *cqe)
 #endif
 	/* Look up the IO */
 	io = ocs_hw_io_lookup(hw, opt_wr->xri);
+	if (io == NULL) {
+		ocs_log_err(hw->os, "auto_xrdy data CQE for invalid xri=%#x\n",
+		    opt_wr->xri);
+		return -1;
+	}
 	ocs_lock(&io->axr_lock);
 	buf = io->axr_buf;
 	buf->data_cqe = 1;

sys/dev/ocs_fc/ocs_ioctl.c

@@ -39,6 +39,7 @@
 #include <sys/systm.h>
 #include <sys/linker.h>
 #include <sys/firmware.h>
+#include <sys/priv.h>
 
 static d_open_t		ocs_open;
 static d_close_t	ocs_close;
@@ -97,6 +98,11 @@ ocs_process_sli_config (ocs_t *ocs, ocs_ioctl_elxu_mbox_t *mcmd, ocs_dma_t *dma)
 				sli4_req_common_read_object_t *rdobj =
 					(sli4_req_common_read_object_t *)sli_config->payload.embed;
 
+				if (mcmd->out_bytes > OCS_MAX_DMA_ALLOC) {
+					device_printf(ocs->dev, "%s: COMMON_READ_OBJECT - %llu exceeds max\n",
+					    __func__, (unsigned long long)mcmd->out_bytes);
+					return EINVAL;
+				}
 				if (ocs_dma_alloc(ocs, dma, mcmd->out_bytes, 4096)) {
 					device_printf(ocs->dev, "%s: COMMON_READ_OBJECT - %lld allocation failed\n",
 							__func__, (unsigned long long)mcmd->out_bytes);
@@ -116,6 +122,17 @@ ocs_process_sli_config (ocs_t *ocs, ocs_ioctl_elxu_mbox_t *mcmd, ocs_dma_t *dma)
 			sli4_req_common_write_object_t *wrobj =
 				(sli4_req_common_write_object_t *)sli_config->payload.embed;
 
+			if (mcmd->in_bytes > wrobj->desired_write_length) {
+				device_printf(ocs->dev, "%s: COMMON_WRITE_OBJECT - in_bytes %llu exceeds desired_write_length %u\n",
+				    __func__, (unsigned long long)mcmd->in_bytes,
+				    wrobj->desired_write_length);
+				return EINVAL;
+			}
+			if (wrobj->desired_write_length > OCS_MAX_DMA_ALLOC) {
+				device_printf(ocs->dev, "%s: COMMON_WRITE_OBJECT - desired_write_length %u exceeds max\n",
+				    __func__, wrobj->desired_write_length);
+				return EINVAL;
+			}
 			if (ocs_dma_alloc(ocs, dma, wrobj->desired_write_length, 4096)) {
 				device_printf(ocs->dev, "%s: COMMON_WRITE_OBJECT - %d allocation failed\n",
 						__func__, wrobj->desired_write_length);
@@ -144,6 +161,11 @@ ocs_process_sli_config (ocs_t *ocs, ocs_ioctl_elxu_mbox_t *mcmd, ocs_dma_t *dma)
 				sli4_req_common_read_object_list_t *rdobj =
 					(sli4_req_common_read_object_list_t *)sli_config->payload.embed;
 
+				if (mcmd->out_bytes > OCS_MAX_DMA_ALLOC) {
+					device_printf(ocs->dev, "%s: COMMON_READ_OBJECT_LIST - %llu exceeds max\n",
+					    __func__, (unsigned long long)mcmd->out_bytes);
+					return EINVAL;
+				}
 				if (ocs_dma_alloc(ocs, dma, mcmd->out_bytes, 4096)) {
 					device_printf(ocs->dev, "%s: COMMON_READ_OBJECT_LIST - %lld allocation failed\n",
 							__func__,(unsigned long long) mcmd->out_bytes);
@@ -170,10 +192,15 @@ ocs_process_sli_config (ocs_t *ocs, ocs_ioctl_elxu_mbox_t *mcmd, ocs_dma_t *dma)
 			break;
 		}
 	} else {
-		uint32_t max_bytes = max(mcmd->in_bytes, mcmd->out_bytes);
+		uint64_t max_bytes = max(mcmd->in_bytes, mcmd->out_bytes);
+		if (max_bytes > OCS_MAX_DMA_ALLOC) {
+			device_printf(ocs->dev, "%s: non-embedded - %llu exceeds max\n",
+			    __func__, (unsigned long long)max_bytes);
+			return EINVAL;
+		}
 		if (ocs_dma_alloc(ocs, dma, max_bytes, 4096)) {
-			device_printf(ocs->dev, "%s: non-embedded - %u allocation failed\n",
-					__func__, max_bytes);
+			device_printf(ocs->dev, "%s: non-embedded - %llu allocation failed\n",
+					__func__, (unsigned long long)max_bytes);
 			return ENXIO;
 		}
 
@@ -210,8 +237,17 @@ ocs_process_mbx_ioctl(ocs_t *ocs, ocs_ioctl_elxu_mbox_t *mcmd)
 
 	switch(((sli4_mbox_command_header_t *)mcmd->payload)->command) {
 	case SLI4_MBOX_COMMAND_SLI_CONFIG:
-		if (ENXIO == ocs_process_sli_config(ocs, mcmd, &dma))
-			return ENXIO;
+		error = ocs_process_sli_config(ocs, mcmd, &dma);
+		if (error != 0)
+			goto no_support;
+		/* never copy out more than was allocated for the command */
+		if (mcmd->out_bytes && dma.virt && mcmd->out_bytes > dma.size) {
+			device_printf(ocs->dev, "%s: out_bytes %llu exceeds buffer size %zu\n",
+			    __func__, (unsigned long long)mcmd->out_bytes,
+			    dma.size);
+			error = EINVAL;
+			goto no_support;
+		}
 		break;
 
 	case SLI4_MBOX_COMMAND_READ_REV:
@@ -299,6 +335,12 @@ ocs_process_ecd_helper (ocs_t *ocs, ocs_ioctl_ecd_helper_t *req)
 			device_printf(ocs->dev, "Error: bar %d not defined\n", req->bar);
 			return -EFAULT;
 		}
+		if ((uint64_t)req->offset + sizeof(uint32_t) >
+		    rman_get_size(ocs->reg[req->bar].res)) {
+			device_printf(ocs->dev, "Error: bar %d offset 0x%x out of range\n",
+				      req->bar, req->offset);
+			return -EFAULT;
+		}
 		break;
 	default:
 		break;
@@ -359,6 +401,11 @@ ocs_ioctl(struct cdev *cdev, u_long cmd, caddr_t addr, int flag, struct thread *
 	struct ocs_softc *ocs = cdev->si_drv1;
 	device_t dev = ocs->dev;
 
+	status = priv_check(td, PRIV_DRIVER);
+	if (status) {
+		return (status);
+	}
+
 	switch (cmd) {
 	case OCS_IOCTL_CMD_ELXU_MBOX: {
 		/* "copyin" done by kernel; thus, just dereference addr */
@@ -1212,7 +1259,7 @@ ocs_debug_attach(void *os)
 	}
 
 	unit = device_get_unit(ocs->dev);
-	ocs->cdev = make_dev(&ocs_cdevsw, unit, UID_ROOT, GID_OPERATOR, 0640,
+	ocs->cdev = make_dev(&ocs_cdevsw, unit, UID_ROOT, GID_WHEEL, 0600,
 			"ocs%d", unit);
 	if (ocs->cdev) {
 		ocs->cdev->si_drv1 = ocs;

sys/dev/ocs_fc/ocs_os.h

@@ -1325,11 +1325,14 @@ ocs_ref_get(ocs_ref_t *ref)
 static inline uint32_t
 ocs_ref_get_unless_zero(ocs_ref_t *ref)
 {
-	uint32_t rc = 0;
+	uint32_t rc;
+
 	rc = ocs_atomic_read(&ref->count);
-		if (rc != 0) {
-			ocs_atomic_add_return(&ref->count, 1);
-		}
+	do {
+		if (rc == 0)
+			return 0;
+	} while (!atomic_fcmpset_32(&ref->count, &rc, rc + 1));
+
 	return rc;
 }
 

sys/dev/ocs_fc/ocs_unsol.c

@@ -788,8 +788,11 @@ static int32_t
 ocs_validate_fcp_cmd(ocs_t *ocs, ocs_hw_sequence_t *seq)
 {
 	size_t		exp_payload_len = 0;
+	size_t		cdb_len;
 	fcp_cmnd_iu_t *cmnd = seq->payload->dma.virt;
-	exp_payload_len = sizeof(fcp_cmnd_iu_t) - 16 + cmnd->additional_fcp_cdb_length;
+	/* additional_fcp_cdb_length is in 4-byte words */
+	exp_payload_len = sizeof(fcp_cmnd_iu_t) - 16 +
+	    cmnd->additional_fcp_cdb_length * sizeof(uint32_t);
 
 	/*
 	 * If we received less than FCP_CMND_IU bytes, assume that the frame is
@@ -803,6 +806,16 @@ ocs_validate_fcp_cmd(ocs_t *ocs, ocs_hw_sequence_t *seq)
 			      exp_payload_len);
 		return -1;
 	}
+
+	/* CAM cannot carry a CDB longer than CAM_MAX_CDBLEN; do not truncate */
+	cdb_len = sizeof(cmnd->fcp_cdb) +
+	    cmnd->additional_fcp_cdb_length * sizeof(uint32_t);
+	if (cmnd->task_management_flags == 0 && cdb_len > CAM_MAX_CDBLEN) {
+		fc_header_t	*fchdr = seq->header->dma.virt;
+		ocs_log_debug(ocs, "dropping ox_id %04x with unsupported CDB length (%zd > %d)\n",
+			    ocs_be16toh(fchdr->ox_id), cdb_len, CAM_MAX_CDBLEN);
+		return -1;
+	}
 	return 0;
 
 }