csiostor: Avoid content leaks and casts

PATCH Applied Kees Cook 3 participants 6 messages May 9, 2017 – May 24, 2017 SCSI

Share & tools

Kees Cook

author

2017-05-09 22:34 UTC #0 lore raw

PATCH 1/1 csiostor: Avoid content leaks and casts

When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.

Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.

Cc Daniel Micay <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

drivers/scsi/csiostor/csio_lnode.c +26 ~~-17~~

index c00b2ff72b55..be5ee2d37815 100644
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
 }
 
 static inline void
-csio_append_attrib(uint8_t **ptr, uint16_t type, uint8_t *val, uint16_t len)
+csio_append_attrib(uint8_t **ptr, uint16_t type, void *val, size_t val_len)
 {
+	uint16_t len;
 	struct fc_fdmi_attr_entry *ae = (struct fc_fdmi_attr_entry *)*ptr;
+
+	if (WARN_ON(val_len > U16_MAX))
+		return;
+
+	len = val_len;
+
 	ae->type = htons(type);
 	len += 4;		/* includes attribute type and length */
 	len = (len + 3) & ~3;	/* should be multiple of 4 bytes */
 	ae->len = htons(len);
-	memcpy(ae->value, val, len);
+	memcpy(ae->value, val, val_len);
+	if (len > val_len)
+		memset(ae->value + val_len, 0, len - val_len);
 	*ptr += len;
 }

@@ -335,7 +344,7 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req)
 	numattrs++;
 	val = htonl(FC_PORTSPEED_1GBIT | FC_PORTSPEED_10GBIT);
 	csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_SUPPORTEDSPEED,
-			   (uint8_t *)&val,
+			   &val,
 			   FC_FDMI_PORT_ATTR_SUPPORTEDSPEED_LEN);
 	numattrs++;

@@ -346,23 +355,22 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req)
 	else
 		val = htonl(CSIO_HBA_PORTSPEED_UNKNOWN);
 	csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED,
-			   (uint8_t *)&val,
-			   FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
+			   &val, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
 	numattrs++;
 
 	mfs = ln->ln_sparm.csp.sp_bb_data;
 	csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_MAXFRAMESIZE,
-			   (uint8_t *)&mfs, FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN);
+			   &mfs, sizeof(mfs));
 	numattrs++;
 
 	strcpy(buf, "csiostor");
 	csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_OSDEVICENAME, buf,
-			   (uint16_t)strlen(buf));
+			   strlen(buf));
 	numattrs++;
 
 	if (!csio_hostname(buf, sizeof(buf))) {
 		csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_HOSTNAME,
-				   buf, (uint16_t)strlen(buf));
+				   buf, strlen(buf));
 		numattrs++;
 	}
 	attrib_blk->numattrs = htonl(numattrs);

@@ -444,33 +452,32 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req)
 
 	strcpy(buf, "Chelsio Communications");
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MANUFACTURER, buf,
-			   (uint16_t)strlen(buf));
+			   strlen(buf));
 	numattrs++;
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_SERIALNUMBER,
-			   hw->vpd.sn, (uint16_t)sizeof(hw->vpd.sn));
+			   hw->vpd.sn, sizeof(hw->vpd.sn));
 	numattrs++;
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODEL, hw->vpd.id,
-			   (uint16_t)sizeof(hw->vpd.id));
+			   sizeof(hw->vpd.id));
 	numattrs++;
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODELDESCRIPTION,
-			   hw->model_desc, (uint16_t)strlen(hw->model_desc));
+			   hw->model_desc, strlen(hw->model_desc));
 	numattrs++;
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_HARDWAREVERSION,
-			   hw->hw_ver, (uint16_t)sizeof(hw->hw_ver));
+			   hw->hw_ver, sizeof(hw->hw_ver));
 	numattrs++;
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_FIRMWAREVERSION,
-			   hw->fwrev_str, (uint16_t)strlen(hw->fwrev_str));
+			   hw->fwrev_str, strlen(hw->fwrev_str));
 	numattrs++;
 
 	if (!csio_osname(buf, sizeof(buf))) {
 		csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_OSNAMEVERSION,
-				   buf, (uint16_t)strlen(buf));
+				   buf, strlen(buf));
 		numattrs++;
 	}
 
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD,
-			   (uint8_t *)&maxpayload,
-			   FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
+			   &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
 	len = (uint32_t)(pld - (uint8_t *)cmd);
 	numattrs++;
 	attrib_blk->numattrs = htonl(numattrs);

@@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req,
 	struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw);
 	int rv;
 
+	BUG_ON(pld_len > pld->len);
+
 	io_req->io_cbfn = io_cbfn;	/* Upper layer callback handler */
 	io_req->fw_handle = (uintptr_t) (io_req);
 	io_req->eq_idx = mgmtm->eq_idx;

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Martin K. Petersen

2017-05-19 01:35 UTC #1 lore raw

Varun,

You weren't CC:ed on this patch. Please review. Thanks!

Quoted 9 lines

When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.

https://patchwork.kernel.org/patch/9718995/

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Varun Prakash

2017-05-22 15:05 UTC #2 lore raw

In reply to Kees Cook

Quoted 34 lines · Kees Cook

When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.
Cc Daniel Micay <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)
drivers/scsi/csiostor/csio_lnode.c +1 -2
index c00b2ff72b55..be5ee2d37815 100644
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
 }
 
 
 
 
 	csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD,
-			   (uint8_t *)&maxpayload,
-			   FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
+			   &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
 	len = (uint32_t)(pld - (uint8_t *)cmd);
 	numattrs++;
 	attrib_blk->numattrs = htonl(numattrs);

drivers/scsi/csiostor/csio_lnode.c csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, · line 1805

Quoted 5 lines

drivers/scsi/csiostor/csio_lnode.c +2 -0

@@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req,
 	struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw);
 	int rv;
 
+	BUG_ON(pld_len > pld->len);
+

I think WARN_ON() is better than BUG_ON() in this case

	if (WARN_ON(pld_len > pld->len))
		return -EINVAL;

Quoted 3 lines

drivers/scsi/csiostor/csio_lnode.c +0 -0

 	io_req->io_cbfn = io_cbfn;	/* Upper layer callback handler */
 	io_req->fw_handle = (uintptr_t) (io_req);
 	io_req->eq_idx = mgmtm->eq_idx;

Kees Cook

author

2017-05-22 16:29 UTC #3 lore raw

Quoted 38 lines · Varun Prakash

Quoted 37 lines · Kees Cook
When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.
Cc Daniel Micay <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)
drivers/scsi/csiostor/csio_lnode.c +1 -2
index c00b2ff72b55..be5ee2d37815 100644
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
 }
 
 
 
 
      csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD,
-                        (uint8_t *)&maxpayload,
-                        FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
+                        &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
      len = (uint32_t)(pld - (uint8_t *)cmd);
      numattrs++;
      attrib_blk->numattrs = htonl(numattrs);
@@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req,
      struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw);
      int rv;
 

Quoted 11 lines

drivers/scsi/csiostor/csio_lnode.c csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, · line 1805
Quoted 2 lines
drivers/scsi/csiostor/csio_lnode.c +2 -0
+     BUG_ON(pld_len > pld->len);
+
I think WARN_ON() is better than BUG_ON() in this case
        if (WARN_ON(pld_len > pld->len))
                return -EINVAL;
Quoted 3 lines
drivers/scsi/csiostor/csio_lnode.c +0 -0
      io_req->io_cbfn = io_cbfn;      /* Upper layer callback handler */
      io_req->fw_handle = (uintptr_t) (io_req);
      io_req->eq_idx = mgmtm->eq_idx;

I chose BUG_ON here because the damage has already been done. If this assertion is hit, the heap buffers have already been overrun. This isn't a state we should only warn about...

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Varun Prakash

Acked-by

2017-05-23 09:30 UTC #4 lore raw

Quoted 47 lines · Kees Cook

Quoted 46 lines · Varun Prakash
Quoted 34 lines · Kees Cook
When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.
Cc Daniel Micay <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
 drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)
drivers/scsi/csiostor/csio_lnode.c +1 -2
index c00b2ff72b55..be5ee2d37815 100644
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
 }
 
 
 
 
      csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD,
-                        (uint8_t *)&maxpayload,
-                        FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
+                        &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN);
      len = (uint32_t)(pld - (uint8_t *)cmd);
      numattrs++;
      attrib_blk->numattrs = htonl(numattrs);
drivers/scsi/csiostor/csio_lnode.c csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, · line 1805
Quoted 5 lines
drivers/scsi/csiostor/csio_lnode.c +2 -0
@@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req,
      struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw);
      int rv;
 
+     BUG_ON(pld_len > pld->len);
+
I think WARN_ON() is better than BUG_ON() in this case
        if (WARN_ON(pld_len > pld->len))
                return -EINVAL;

Quoted 8 lines

Quoted 3 lines
drivers/scsi/csiostor/csio_lnode.c csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, · line 1808
Quoted 3 lines
drivers/scsi/csiostor/csio_lnode.c +0 -0
      io_req->io_cbfn = io_cbfn;      /* Upper layer callback handler */
      io_req->fw_handle = (uintptr_t) (io_req);
      io_req->eq_idx = mgmtm->eq_idx;
I chose BUG_ON here because the damage has already been done. If this assertion is hit, the heap buffers have already been overrun. This isn't a state we should only warn about...

Ok.

Acked-by Varun Prakash <[email protected]>

Martin K. Petersen

Applied

2017-05-24 01:46 UTC #5 lore raw

In reply to Kees Cook

Kees,

Quoted 9 lines

When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks.

Applied to 4.13/scsi-queue, thanks!

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering