diff mbox

powerpc/pseries: Correct possible read beyond dlpar sysfs buffer

Message ID 20161117163810.7355.74279.stgit@ltcalpine2-lp14.aus.stglabs.ibm.com (mailing list archive)
State Accepted
Headers show

Commit Message

Nathan Fontenot Nov. 17, 2016, 4:38 p.m. UTC
The pasrsing of data written to the dlpar file in sysfs does not correctly
account for the possibility of reading past the end of the buffer. The code
assumes that all pieces of the command witten to the sysfs file are present
in the form "<resource> <action> <id_type> <id>".

Correct this by updating the buffer parsing code to make a local copy and
use the strsep() and sysfs_streq() routines to parse the buffer. This patch
also separates the parsing code into subroutines for each piece of the
command.

Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/dlpar.c |  141 ++++++++++++++++++++++----------
 1 file changed, 96 insertions(+), 45 deletions(-)

Comments

Michael Ellerman Nov. 22, 2016, 12:34 a.m. UTC | #1
On Thu, 2016-11-17 at 16:38:10 UTC, Nathan Fontenot wrote:
> The pasrsing of data written to the dlpar file in sysfs does not correctly
> account for the possibility of reading past the end of the buffer. The code
> assumes that all pieces of the command witten to the sysfs file are present
> in the form "<resource> <action> <id_type> <id>".
> 
> Correct this by updating the buffer parsing code to make a local copy and
> use the strsep() and sysfs_streq() routines to parse the buffer. This patch
> also separates the parsing code into subroutines for each piece of the
> command.
> 
> Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/25b587fba9a4198e7ea2e2319d62e2

cheers
diff mbox

Patch

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 423e450..ec5d677 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -418,84 +418,135 @@  void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
 	}
 }
 
-static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
-			   const char *buf, size_t count)
+static int dlpar_parse_resource(char **cmd, struct pseries_hp_errorlog *hp_elog)
 {
-	struct pseries_hp_errorlog *hp_elog;
-	struct completion hotplug_done;
-	const char *arg;
-	int rc;
+	char *arg;
 
-	hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
-	if (!hp_elog) {
-		rc = -ENOMEM;
-		goto dlpar_store_out;
-	}
+	arg = strsep(cmd, " ");
+	if (!arg)
+		return -EINVAL;
 
-	/* Parse out the request from the user, this will be in the form
-	 * <resource> <action> <id_type> <id>
-	 */
-	arg = buf;
-	if (!strncmp(arg, "memory", 6)) {
+	if (sysfs_streq(arg, "memory")) {
 		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_MEM;
-		arg += strlen("memory ");
-	} else if (!strncmp(arg, "cpu", 3)) {
+	} else if (sysfs_streq(arg, "cpu")) {
 		hp_elog->resource = PSERIES_HP_ELOG_RESOURCE_CPU;
-		arg += strlen("cpu ");
 	} else {
-		pr_err("Invalid resource specified: \"%s\"\n", buf);
-		rc = -EINVAL;
-		goto dlpar_store_out;
+		pr_err("Invalid resource specified.\n");
+		return -EINVAL;
 	}
 
-	if (!strncmp(arg, "add", 3)) {
+	return 0;
+}
+
+static int dlpar_parse_action(char **cmd, struct pseries_hp_errorlog *hp_elog)
+{
+	char *arg;
+
+	arg = strsep(cmd, " ");
+	if (!arg)
+		return -EINVAL;
+
+	if (sysfs_streq(arg, "add")) {
 		hp_elog->action = PSERIES_HP_ELOG_ACTION_ADD;
-		arg += strlen("add ");
-	} else if (!strncmp(arg, "remove", 6)) {
+	} else if (sysfs_streq(arg, "remove")) {
 		hp_elog->action = PSERIES_HP_ELOG_ACTION_REMOVE;
-		arg += strlen("remove ");
 	} else {
-		pr_err("Invalid action specified: \"%s\"\n", buf);
-		rc = -EINVAL;
-		goto dlpar_store_out;
+		pr_err("Invalid action specified.\n");
+		return -EINVAL;
 	}
 
-	if (!strncmp(arg, "index", 5)) {
-		u32 index;
+	return 0;
+}
 
+static int dlpar_parse_id_type(char **cmd, struct pseries_hp_errorlog *hp_elog)
+{
+	char *arg;
+	u32 count, index;
+
+	arg = strsep(cmd, " ");
+	if (!arg)
+		return -EINVAL;
+
+	if (sysfs_streq(arg, "index")) {
 		hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
-		arg += strlen("index ");
+		arg = strsep(cmd, " ");
+		if (!arg) {
+			pr_err("No DRC Index specified.\n");
+			return -EINVAL;
+		}
+
 		if (kstrtou32(arg, 0, &index)) {
-			rc = -EINVAL;
-			pr_err("Invalid drc_index specified: \"%s\"\n", buf);
-			goto dlpar_store_out;
+			pr_err("Invalid DRC Index specified.\n");
+			return -EINVAL;
 		}
 
 		hp_elog->_drc_u.drc_index = cpu_to_be32(index);
-	} else if (!strncmp(arg, "count", 5)) {
-		u32 count;
-
+	} else if (sysfs_streq(arg, "count")) {
 		hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_COUNT;
-		arg += strlen("count ");
+		arg = strsep(cmd, " ");
+		if (!arg) {
+			pr_err("No DRC count specified.\n");
+			return -EINVAL;
+		}
+
 		if (kstrtou32(arg, 0, &count)) {
-			rc = -EINVAL;
-			pr_err("Invalid count specified: \"%s\"\n", buf);
-			goto dlpar_store_out;
+			pr_err("Invalid DRC count specified.\n");
+			return -EINVAL;
 		}
 
 		hp_elog->_drc_u.drc_count = cpu_to_be32(count);
 	} else {
-		pr_err("Invalid id_type specified: \"%s\"\n", buf);
-		rc = -EINVAL;
-		goto dlpar_store_out;
+		pr_err("Invalid id_type specified.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t dlpar_store(struct class *class, struct class_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct pseries_hp_errorlog *hp_elog;
+	struct completion hotplug_done;
+	char *argbuf;
+	char *args;
+	int rc;
+
+	args = argbuf = kstrdup(buf, GFP_KERNEL);
+	hp_elog = kzalloc(sizeof(*hp_elog), GFP_KERNEL);
+	if (!hp_elog || !argbuf) {
+		pr_info("Could not allocate resources for DLPAR operation\n");
+		kfree(argbuf);
+		kfree(hp_elog);
+		return -ENOMEM;
 	}
 
+	/* Parse out the request from the user, this will be in the form
+	 * <resource> <action> <id_type> <id>
+	 */
+	rc = dlpar_parse_resource(&args, hp_elog);
+	if (rc)
+		goto dlpar_store_out;
+
+	rc = dlpar_parse_action(&args, hp_elog);
+	if (rc)
+		goto dlpar_store_out;
+
+	rc = dlpar_parse_id_type(&args, hp_elog);
+	if (rc)
+		goto dlpar_store_out;
+
 	init_completion(&hotplug_done);
 	queue_hotplug_event(hp_elog, &hotplug_done, &rc);
 	wait_for_completion(&hotplug_done);
 
 dlpar_store_out:
+	kfree(argbuf);
 	kfree(hp_elog);
+
+	if (rc)
+		pr_err("Could not handle DLPAR request \"%s\"\n", buf);
+
 	return rc ? rc : count;
 }