ipr: Fix stack overflow in ipr_format_resource_path

Victor reported an oops during boot with 2.6.34 on a POWER6 JS22:

https://bugzilla.kernel.org/show_bug.cgi?id=16089

Checking ipr microcode levels

Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0x322d30312d31302c
...
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=128 NUMA pSeries
last sysfs file:
/sys/devices/pci0000:00/0000:00:01.0/host0/target0:255:255/0:255:255:255/resource_path
Modules linked in:
NIP: 322d30312d31302c LR: 322d30312d31302d CTR: c000000000375bec
REGS: c0000003d360f8f0 TRAP: 0400   Not tainted  (2.6.34-vjj)
MSR: 8000000040009432 <EE,ME,IR,DR>  CR: 28002484  XER: 20000020

TASK = c0000003d587d010[5163] 'iprupdate' THREAD: c0000003d360c000 CPU: 7
GPR00: 322d30312d31302d c0000003d360fb70 c000000000ad07c0 00000000000185a0
GPR04: 0000000000000001 c0000003d360fb10 04000affffffffff c0000000006a6700
GPR08: c000000000823383 0000000000000000 0000000000000020 0000000000000000
GPR12: 000000000000f032 c00000000f622e00 00000000000000ed 0000000000000000
GPR16: 00000000100b8808 0000000010020000 0000000010020000 0000000010010000
GPR20: 0000000010010000 0000000000000001 0000000000001000 000000001045eef8
GPR24: c0000003d360fdf8 302d31342d31302d 43302d30302d3030 2d30332d44352d34
GPR28: 422d33382d30302d 30302d30302d3030 2d30302d30302d30 302d30302d30302d NIP [322d30312d31302c] 0x322d30312d31302c
LR [322d30312d31302d] 0x322d30312d31302d

A stack overflow. It turns out ipr_format_resource_path writes to a passed in buffer using sprintf which is dangerous and in this case looks to be the cause of the overflow.

The following patch passes in the length of the buffer and uses snprintf, which fixes the fail for me. It doesn't fix the other issue, which is why the loop in ipr_format_resource_path didn't terminate in the first place. That can be fixed in a follow up patch.

Signed-off-by Anton Blanchard <[email protected]>

---

Index: linux.trees.git/drivers/scsi/ipr.c
===================================================================

drivers/scsi/ipr.c +32 -14

@@ -1132,17 +1132,20 @@ static int ipr_is_same_device(struct ipr
  * ipr_format_resource_path - Format the resource path for printing.
  * @res_path:	resource path
  * @buf:	buffer
+ * @len:	length of the buffer
  *
  * Return value:
  * 	pointer to buffer
  **/
-static char *ipr_format_resource_path(u8 *res_path, char *buffer)
+static char *ipr_format_resource_path(u8 *res_path, char *buffer,
+				      unsigned int len)
 {
 	int i;
+	char *p = buffer;
 
-	sprintf(buffer, "%02X", res_path[0]);
+	p += snprintf(p, buffer + len - p, "%02X", res_path[0]);
 	for (i=1; res_path[i] != 0xff; i++)
-		sprintf(buffer, "%s-%02X", buffer, res_path[i]);
+		p += snprintf(p, buffer + len - p, "-%02X", res_path[i]);
 
 	return buffer;
 }

@@ -1187,7 +1190,8 @@ static void ipr_update_res_entry(struct 
 
 		if (res->sdev && new_path)
 			sdev_printk(KERN_INFO, res->sdev, "Resource path: %s\n",
-				    ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+				    ipr_format_resource_path(&res->res_path[0],
+				    &buffer[0], sizeof(buffer)));
 	} else {
 		res->flags = cfgtew->u.cfgte->flags;
 		if (res->flags & IPR_IS_IOA_RESOURCE)

@@ -1573,7 +1577,8 @@ static void ipr_log_sis64_config_error(s
 		ipr_err_separator;
 
 		ipr_err("Device %d : %s", i + 1,
-			 ipr_format_resource_path(&dev_entry->res_path[0], &buffer[0]));
+			 ipr_format_resource_path(&dev_entry->res_path[0],
+			 &buffer[0], sizeof(buffer)));
 		ipr_log_ext_vpd(&dev_entry->vpd);
 
 		ipr_err("-----New Device Information-----\n");

@@ -1919,13 +1924,15 @@ static void ipr_log64_fabric_path(struct
 
 			ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s\n",
 				     path_active_desc[i].desc, path_state_desc[j].desc,
-				     ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
+				     ipr_format_resource_path(&fabric->res_path[0],
+				     &buffer[0], sizeof(buffer)));
 			return;
 		}
 	}
 
 	ipr_err("Path state=%02X Resource Path=%s\n", path_state,
-		ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
+		ipr_format_resource_path(&fabric->res_path[0], &buffer[0],
+		sizeof(buffer)));
 }
 
 static const struct {

@@ -2066,7 +2073,9 @@ static void ipr_log64_path_elem(struct i
 
 			ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s, Link rate=%s, WWN=%08X%08X\n",
 				     path_status_desc[j].desc, path_type_desc[i].desc,
-				     ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
+				     ipr_format_resource_path(&cfg->res_path[0],
+							      &buffer[0],
+							      sizeof(buffer)),
 				     link_rate[cfg->link_rate & IPR_PHY_LINK_RATE_MASK],
 				     be32_to_cpu(cfg->wwid[0]), be32_to_cpu(cfg->wwid[1]));
 			return;

@@ -2074,7 +2083,8 @@ static void ipr_log64_path_elem(struct i
 	}
 	ipr_hcam_err(hostrcb, "Path element=%02X: Resource Path=%s, Link rate=%s "
 		     "WWN=%08X%08X\n", cfg->type_status,
-		     ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
+		     ipr_format_resource_path(&cfg->res_path[0], &buffer[0],
+		     sizeof(buffer)),
 		     link_rate[cfg->link_rate & IPR_PHY_LINK_RATE_MASK],
 		     be32_to_cpu(cfg->wwid[0]), be32_to_cpu(cfg->wwid[1]));
 }

@@ -2139,7 +2149,8 @@ static void ipr_log_sis64_array_error(st
 
 	ipr_err("RAID %s Array Configuration: %s\n",
 		error->protection_level,
-		ipr_format_resource_path(&error->last_res_path[0], &buffer[0]));
+		ipr_format_resource_path(&error->last_res_path[0], &buffer[0],
+					 sizeof(buffer)));
 
 	ipr_err_separator;
 

@@ -2160,9 +2171,12 @@ static void ipr_log_sis64_array_error(st
 		ipr_err("Array Member %d:\n", i);
 		ipr_log_ext_vpd(&array_entry->vpd);
 		ipr_err("Current Location: %s",
-			 ipr_format_resource_path(&array_entry->res_path[0], &buffer[0]));
+			 ipr_format_resource_path(&array_entry->res_path[0],
+						  &buffer[0],
+						  sizeof(buffer)));
 		ipr_err("Expected Location: %s",
-			 ipr_format_resource_path(&array_entry->expected_res_path[0], &buffer[0]));
+			 ipr_format_resource_path(&array_entry->expected_res_path[0],
+						  &buffer[0], sizeof(buffer)));
 
 		ipr_err_separator;
 	}

@@ -4099,7 +4113,9 @@ static ssize_t ipr_show_resource_path(st
 	res = (struct ipr_resource_entry *)sdev->hostdata;
 	if (res)
 		len = snprintf(buf, PAGE_SIZE, "%s\n",
-			       ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+			       ipr_format_resource_path(&res->res_path[0],
+							&buffer[0],
+							sizeof(buffer)));
 	spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
 	return len;
 }

@@ -4351,7 +4367,9 @@ static int ipr_slave_configure(struct sc
 			scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 		if (ioa_cfg->sis64)
 			sdev_printk(KERN_INFO, sdev, "Resource path: %s\n",
-			            ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+			            ipr_format_resource_path(&res->res_path[0],
+							     &buffer[0],
+							     sizeof(buffer)));
 		return 0;
 	}
 	spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);

Index: linux.trees.git/drivers/scsi/ipr.h
===================================================================

drivers/scsi/ipr.h +2 -2

@@ -1685,7 +1685,8 @@ struct ipr_ucode_image_header {
 		if ((hostrcb)->ioa_cfg->sis64) {			\
 			printk(KERN_ERR IPR_NAME ": %s: " fmt, 		\
 				ipr_format_resource_path(&hostrcb->hcam.u.error64.fd_res_path[0], \
-					&hostrcb->rp_buffer[0]),	\
+					&hostrcb->rp_buffer[0],		\
+					sizeof(hostrcb->rp_buffer)),	\
 				__VA_ARGS__);				\
 		} else {						\
 			ipr_ra_err((hostrcb)->ioa_cfg,			\
--

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

We also saw a variation of this problem last week and I have an alternative patch that I'd prefer over this one. I'll post the patch in a separate email.

It was possible to overflow the buffer used to print out the formatted version of the resource path. The fix is to limit the number of bytes that get formatted.

This patch also updates the ipr_show_resource_path function to display the resource address for devices that are attached to adapters that don't support resource paths.

Signed-off-by Wayne Boyer <[email protected]>

---

James, this patch needs to go into rc-fixes. It fixes an oops that is currently being seen on Power.

---
 drivers/scsi/ipr.c |   51 +++++++++++++++++++++++++++++++++------------------
 drivers/scsi/ipr.h |    5 +++--
 2 files changed, 36 insertions(+), 20 deletions(-)

Index: b/drivers/scsi/ipr.c
===================================================================

drivers/scsi/ipr.c +33 -18

@@ -1129,20 +1129,22 @@ static int ipr_is_same_device(struct ipr
 }
 
 /**
- * ipr_format_resource_path - Format the resource path for printing.
+ * ipr_format_res_path - Format the resource path for printing.
  * @res_path:	resource path
  * @buf:	buffer
  *
  * Return value:
  * 	pointer to buffer
  **/
-static char *ipr_format_resource_path(u8 *res_path, char *buffer)
+static char *ipr_format_res_path(u8 *res_path, char *buffer, int len)
 {
 	int i;
+	char *p = buffer;
 
-	sprintf(buffer, "%02X", res_path[0]);
-	for (i=1; res_path[i] != 0xff; i++)
-		sprintf(buffer, "%s-%02X", buffer, res_path[i]);
+	res_path[0] = '\0';
+	p += snprintf(p, buffer + len - p, "%02X", res_path[0]);
+	for (i = 1; res_path[i] != 0xff && ((i * 3) < len); i++)
+		p += snprintf(p, buffer + len - p, "-%02X", res_path[i]);
 
 	return buffer;
 }

@@ -1187,7 +1189,8 @@ static void ipr_update_res_entry(struct 
 
 		if (res->sdev && new_path)
 			sdev_printk(KERN_INFO, res->sdev, "Resource path: %s\n",
-				    ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+				    ipr_format_res_path(res->res_path, buffer,
+							sizeof(buffer)));
 	} else {
 		res->flags = cfgtew->u.cfgte->flags;
 		if (res->flags & IPR_IS_IOA_RESOURCE)

@@ -1573,7 +1576,8 @@ static void ipr_log_sis64_config_error(s
 		ipr_err_separator;
 
 		ipr_err("Device %d : %s", i + 1,
-			 ipr_format_resource_path(&dev_entry->res_path[0], &buffer[0]));
+			 ipr_format_res_path(dev_entry->res_path, buffer,
+					     sizeof(buffer)));
 		ipr_log_ext_vpd(&dev_entry->vpd);
 
 		ipr_err("-----New Device Information-----\n");

@@ -1919,13 +1923,14 @@ static void ipr_log64_fabric_path(struct
 
 			ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s\n",
 				     path_active_desc[i].desc, path_state_desc[j].desc,
-				     ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
+				     ipr_format_res_path(fabric->res_path, buffer,
+							 sizeof(buffer)));
 			return;
 		}
 	}
 
 	ipr_err("Path state=%02X Resource Path=%s\n", path_state,
-		ipr_format_resource_path(&fabric->res_path[0], &buffer[0]));
+		ipr_format_res_path(fabric->res_path, buffer, sizeof(buffer)));
 }
 
 static const struct {

@@ -2066,7 +2071,8 @@ static void ipr_log64_path_elem(struct i
 
 			ipr_hcam_err(hostrcb, "%s %s: Resource Path=%s, Link rate=%s, WWN=%08X%08X\n",
 				     path_status_desc[j].desc, path_type_desc[i].desc,
-				     ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
+				     ipr_format_res_path(cfg->res_path, buffer,
+							 sizeof(buffer)),
 				     link_rate[cfg->link_rate & IPR_PHY_LINK_RATE_MASK],
 				     be32_to_cpu(cfg->wwid[0]), be32_to_cpu(cfg->wwid[1]));
 			return;

@@ -2074,7 +2080,7 @@ static void ipr_log64_path_elem(struct i
 	}
 	ipr_hcam_err(hostrcb, "Path element=%02X: Resource Path=%s, Link rate=%s "
 		     "WWN=%08X%08X\n", cfg->type_status,
-		     ipr_format_resource_path(&cfg->res_path[0], &buffer[0]),
+		     ipr_format_res_path(cfg->res_path, buffer, sizeof(buffer)),
 		     link_rate[cfg->link_rate & IPR_PHY_LINK_RATE_MASK],
 		     be32_to_cpu(cfg->wwid[0]), be32_to_cpu(cfg->wwid[1]));
 }

@@ -2139,7 +2145,7 @@ static void ipr_log_sis64_array_error(st
 
 	ipr_err("RAID %s Array Configuration: %s\n",
 		error->protection_level,
-		ipr_format_resource_path(&error->last_res_path[0], &buffer[0]));
+		ipr_format_res_path(error->last_res_path, buffer, sizeof(buffer)));
 
 	ipr_err_separator;
 

@@ -2160,9 +2166,11 @@ static void ipr_log_sis64_array_error(st
 		ipr_err("Array Member %d:\n", i);
 		ipr_log_ext_vpd(&array_entry->vpd);
 		ipr_err("Current Location: %s",
-			 ipr_format_resource_path(&array_entry->res_path[0], &buffer[0]));
+			 ipr_format_res_path(array_entry->res_path, buffer,
+					     sizeof(buffer)));
 		ipr_err("Expected Location: %s",
-			 ipr_format_resource_path(&array_entry->expected_res_path[0], &buffer[0]));
+			 ipr_format_res_path(array_entry->expected_res_path,
+					     buffer, sizeof(buffer)));
 
 		ipr_err_separator;
 	}

@@ -4076,7 +4084,8 @@ static struct device_attribute ipr_adapt
 };
 
 /**
- * ipr_show_resource_path - Show the resource path for this device.
+ * ipr_show_resource_path - Show the resource path or the resource address for
+ *			    this device.
  * @dev:	device struct
  * @buf:	buffer
  *

@@ -4094,9 +4103,14 @@ static ssize_t ipr_show_resource_path(st
 
 	spin_lock_irqsave(ioa_cfg->host->host_lock, lock_flags);
 	res = (struct ipr_resource_entry *)sdev->hostdata;
-	if (res)
+	if (res && ioa_cfg->sis64)
 		len = snprintf(buf, PAGE_SIZE, "%s\n",
-			       ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+			       ipr_format_res_path(res->res_path, buffer,
+						   sizeof(buffer)));
+	else if (res)
+		len = snprintf(buf, PAGE_SIZE, "%d:%d:%d:%d\n", ioa_cfg->host->host_no,
+			       res->bus, res->target, res->lun);
+
 	spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
 	return len;
 }

@@ -4348,7 +4362,8 @@ static int ipr_slave_configure(struct sc
 			scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 		if (ioa_cfg->sis64)
 			sdev_printk(KERN_INFO, sdev, "Resource path: %s\n",
-			            ipr_format_resource_path(&res->res_path[0], &buffer[0]));
+				    ipr_format_res_path(res->res_path, buffer,
+							sizeof(buffer)));
 		return 0;
 	}
 	spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);

Index: b/drivers/scsi/ipr.h
===================================================================

drivers/scsi/ipr.h +3 -2

@@ -1684,8 +1684,9 @@ struct ipr_ucode_image_header {
 	if (ipr_is_device(hostrcb)) {					\
 		if ((hostrcb)->ioa_cfg->sis64) {			\
 			printk(KERN_ERR IPR_NAME ": %s: " fmt, 		\
-				ipr_format_resource_path(&hostrcb->hcam.u.error64.fd_res_path[0], \
-					&hostrcb->rp_buffer[0]),	\
+				ipr_format_res_path(hostrcb->hcam.u.error64.fd_res_path, \
+					hostrcb->rp_buffer,		\
+					sizeof(hostrcb->rp_buffer)),	\
 				__VA_ARGS__);				\
 		} else {						\
 			ipr_ra_err((hostrcb)->ioa_cfg,			\