[PATCH 0/2] powerpc/pseries: RTAS mobility fixes

[PATCH 0/2] powerpc/pseries: RTAS mobility fixes

[Cyril Bur]

This patchset addresses endian issues and bugs in device tree update for
ibm,update-nodes and ibm,update-properties RTAS calls.

A subseqent patchset will deal with issues in device tree node addition
(ibm,configure-connector RTAS call) as well as more robust handling of
deleting critical device tree nodes.

Cyril Bur (2):
  powerpc/pseries: fix endian bugs in mobility RTAS calls
  powerpc/pseries: fix bugs in RTAS mobility code

 arch/powerpc/platforms/pseries/mobility.c | 143 +++++++++++++++++++-----------
 1 file changed, 89 insertions(+), 54 deletions(-)

-- 
1.9.1

[PATCH 1/2] powerpc/pseries: fix endian bugs in mobility RTAS calls

[Cyril Bur]

These calls use a buffer shared memory buffer to comunicate device tree
updates.

PAPR specifies that RTAS buffers are to be written in big endian.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 50 ++++++++++++++++---------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index e7cb6d4..09bef23 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -40,7 +40,7 @@ struct update_props_workarea {
 
 #define MIGRATION_SCOPE	(1)
 
-static int mobility_rtas_call(int token, char *buf, s32 scope)
+static int mobility_rtas_call(int token, __be32 *buf, s32 scope)
 {
 	int rc;
 
@@ -129,14 +129,14 @@ static int update_dt_property(struct device_node *dn, struct property **prop,
 
 static int update_dt_node(u32 phandle, s32 scope)
 {
-	struct update_props_workarea *upwa;
+	struct update_props_workarea upwa;
 	struct device_node *dn;
 	struct property *prop = NULL;
 	int i, rc, rtas_rc;
-	char *prop_data;
-	char *rtas_buf;
 	int update_properties_token;
+	char *prop_data;
 	u32 vd;
+	__be32 *rtas_buf;
 
 	update_properties_token = rtas_token("ibm,update-properties");
 	if (update_properties_token == RTAS_UNKNOWN_SERVICE)
@@ -152,16 +152,17 @@ static int update_dt_node(u32 phandle, s32 scope)
 		return -ENOENT;
 	}
 
-	upwa = (struct update_props_workarea *)&rtas_buf[0];
-	upwa->phandle = phandle;
-
+	*rtas_buf = cpu_to_be32(phandle);
 	do {
 		rtas_rc = mobility_rtas_call(update_properties_token, rtas_buf,
 					scope);
 		if (rtas_rc < 0)
 			break;
-
-		prop_data = rtas_buf + sizeof(*upwa);
+		upwa.phandle = be32_to_cpu(*rtas_buf);
+		upwa.state = be32_to_cpu(*(rtas_buf + 1));
+		upwa.reserved = be64_to_cpu(*((__be64 *)(rtas_buf + 2)));
+		upwa.nprops = be32_to_cpu(*(rtas_buf + 4));
+		prop_data = ((char *)rtas_buf) + sizeof(upwa);
 
 		/* On the first call to ibm,update-properties for a node the
 		 * the first property value descriptor contains an empty
@@ -169,18 +170,18 @@ static int update_dt_node(u32 phandle, s32 scope)
 		 * and the property value is the node path being updated.
 		 */
 		if (*prop_data == 0) {
-			prop_data++;
-			vd = *(u32 *)prop_data;
+			prop_data += sizeof(u32);
+			vd = be32_to_cpu(*(__be32 *)prop_data);
 			prop_data += vd + sizeof(vd);
-			upwa->nprops--;
+			upwa.nprops--;
 		}
 
-		for (i = 0; i < upwa->nprops; i++) {
+		for (i = 0; i < upwa.nprops; i++) {
 			char *prop_name;
 
 			prop_name = prop_data;
 			prop_data += strlen(prop_name) + 1;
-			vd = *(u32 *)prop_data;
+			vd = be32_to_cpu(*(__be32 *)prop_data);
 			prop_data += sizeof(vd);
 
 			switch (vd) {
@@ -236,10 +237,11 @@ static int add_dt_node(u32 parent_phandle, u32 drc_index)
 
 int pseries_devicetree_update(s32 scope)
 {
-	char *rtas_buf;
-	u32 *data;
+	__be32 *rtas_buf;
 	int update_nodes_token;
 	int rc;
+	__be32 *data;
+	u32 node;
 
 	update_nodes_token = rtas_token("ibm,update-nodes");
 	if (update_nodes_token == RTAS_UNKNOWN_SERVICE)
@@ -253,17 +255,16 @@ int pseries_devicetree_update(s32 scope)
 		rc = mobility_rtas_call(update_nodes_token, rtas_buf, scope);
 		if (rc && rc != 1)
 			break;
+		data = rtas_buf + 4;
+		node = be32_to_cpu(*data++);
 
-		data = (u32 *)rtas_buf + 4;
-		while (*data & NODE_ACTION_MASK) {
+		while (node & NODE_ACTION_MASK) {
 			int i;
-			u32 action = *data & NODE_ACTION_MASK;
-			int node_count = *data & NODE_COUNT_MASK;
-
-			data++;
+			u32 action = node & NODE_ACTION_MASK;
+			int node_count = node & NODE_COUNT_MASK;
 
 			for (i = 0; i < node_count; i++) {
-				u32 phandle = *data++;
+				u32 phandle = be32_to_cpu(*data++);
 				u32 drc_index;
 
 				switch (action) {
@@ -274,11 +275,12 @@ int pseries_devicetree_update(s32 scope)
 					update_dt_node(phandle, scope);
 					break;
 				case ADD_DT_NODE:
-					drc_index = *data++;
+					drc_index = be32_to_cpu(*data++);
 					add_dt_node(phandle, drc_index);
 					break;
 				}
 			}
+			node = be32_to_cpu(*data++);
 		}
 	} while (rc == 1);
 
-- 
1.9.1

[PATCH 2/2] powerpc/pseries: fix bugs in RTAS mobility code

[Cyril Bur]

Running this code on a little endian machine has exposed some very unlikely
corner cases. Most of these oversights will lead to a buffer overflow.

Reworked some of the error pathes. It seems more sane to stop trying to parse
a buffer on errors. Attempting to continue opens up the possibility of
overflows and/or garbage reads.

Don't warn about failed allcations when the amount was taken from the buffer,
assume the value was incorrect, don't needlessly concern the user.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 arch/powerpc/platforms/pseries/mobility.c | 95 +++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 09bef23..00bd939 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -68,62 +68,45 @@ static int delete_dt_node(u32 phandle)
 }
 
 static int update_dt_property(struct device_node *dn, struct property **prop,
-			      const char *name, u32 vd, char *value)
+			      const char *name, int length, char *value)
 {
 	struct property *new_prop = *prop;
-	int more = 0;
-
-	/* A negative 'vd' value indicates that only part of the new property
-	 * value is contained in the buffer and we need to call
-	 * ibm,update-properties again to get the rest of the value.
-	 *
-	 * A negative value is also the two's compliment of the actual value.
-	 */
-	if (vd & 0x80000000) {
-		vd = ~vd + 1;
-		more = 1;
-	}
 
 	if (new_prop) {
 		/* partial property fixup */
-		char *new_data = kzalloc(new_prop->length + vd, GFP_KERNEL);
+		char *new_data = kzalloc(new_prop->length + length, GFP_KERNEL | __GFP_NOWARN);
 		if (!new_data)
 			return -ENOMEM;
 
 		memcpy(new_data, new_prop->value, new_prop->length);
-		memcpy(new_data + new_prop->length, value, vd);
+		memcpy(new_data + new_prop->length, value, length);
 
 		kfree(new_prop->value);
 		new_prop->value = new_data;
-		new_prop->length += vd;
+		new_prop->length += length;
 	} else {
 		new_prop = kzalloc(sizeof(*new_prop), GFP_KERNEL);
 		if (!new_prop)
 			return -ENOMEM;
 
-		new_prop->name = kstrdup(name, GFP_KERNEL);
+		new_prop->name = kstrdup(name, GFP_KERNEL | __GFP_NOWARN);
 		if (!new_prop->name) {
 			kfree(new_prop);
 			return -ENOMEM;
 		}
 
-		new_prop->length = vd;
-		new_prop->value = kzalloc(new_prop->length, GFP_KERNEL);
+		new_prop->length = length;
+		new_prop->value = kzalloc(new_prop->length, GFP_KERNEL | __GFP_NOWARN);
 		if (!new_prop->value) {
 			kfree(new_prop->name);
 			kfree(new_prop);
 			return -ENOMEM;
 		}
 
-		memcpy(new_prop->value, value, vd);
+		memcpy(new_prop->value, value, length);
 		*prop = new_prop;
 	}
 
-	if (!more) {
-		of_update_property(dn, new_prop);
-		*prop = NULL;
-	}
-
 	return 0;
 }
 
@@ -196,21 +179,52 @@ static int update_dt_node(u32 phandle, s32 scope)
 				break;
 
 			default:
+				/* A negative 'vd' value indicates that only part of the new property
+				 * value is contained in the buffer and we need to call
+				 * ibm,update-properties again to get the rest of the value.
+				 *
+				 * A negative value is also the two's compliment of the actual value.
+				 */
+
 				rc = update_dt_property(dn, &prop, prop_name,
-							vd, prop_data);
+							vd & 0x80000000 ? ~vd + 1 : vd, prop_data);
 				if (rc) {
-					printk(KERN_ERR "Could not update %s"
-					       " property\n", prop_name);
+					printk(KERN_ERR "Could not update %s property\n",
+					       prop_name);
+					/* Could try to continue but if the failure was for a section
+					 * of a node it gets too easy to mess up the device tree.
+					 * Plus, ENOMEM likely means we have bigger problems than a
+					 * failed device tree update */
+					if (prop) {
+						kfree(prop->name);
+						kfree(prop->value);
+						kfree(prop);
+						prop = NULL;
+					}
+					i = upwa.nprops - 1; /* Break */
+				}
+
+				if (prop && !(vd & 0x80000000)) {
+					of_update_property(dn, prop);
+					prop = NULL;
 				}
+				prop_data += vd & 0x80000000 ? ~vd + 1 : vd;
+			}
 
-				prop_data += vd;
+			if (prop_data - (char *)rtas_buf >= RTAS_DATA_BUF_SIZE) {
+				printk(KERN_ERR "Device tree property"
+				       " length exceeds rtas buffer\n");
+				rc = -EOVERFLOW;
+				goto update_dt_node_err;
 			}
 		}
 	} while (rtas_rc == 1);
 
+	rc = rtas_rc;
 	of_node_put(dn);
+update_dt_node_err:
 	kfree(rtas_buf);
-	return 0;
+	return rc;
 }
 
 static int add_dt_node(u32 parent_phandle, u32 drc_index)
@@ -264,9 +278,17 @@ int pseries_devicetree_update(s32 scope)
 			int node_count = node & NODE_COUNT_MASK;
 
 			for (i = 0; i < node_count; i++) {
-				u32 phandle = be32_to_cpu(*data++);
+				u32 phandle;
 				u32 drc_index;
 
+				if (data + 1 - rtas_buf >= RTAS_DATA_BUF_SIZE) {
+					printk(KERN_ERR "Device tree property"
+					       " length exceeds rtas buffer\n");
+					rc = -EOVERFLOW;
+					goto pseries_devicetree_update_err;
+				}
+				phandle = be32_to_cpu(*data++);
+
 				switch (action) {
 				case DELETE_DT_NODE:
 					delete_dt_node(phandle);
@@ -278,12 +300,23 @@ int pseries_devicetree_update(s32 scope)
 					drc_index = be32_to_cpu(*data++);
 					add_dt_node(phandle, drc_index);
 					break;
+				default:
+					/* Bogus action */
+					i = node_count - 1; /* Break */
+					data += node_count;
 				}
 			}
+			if (data - rtas_buf >= RTAS_DATA_BUF_SIZE) {
+				printk(KERN_ERR "Number of device tree update "
+				       "nodes exceeds rtas buffer length\n");
+				rc = -EOVERFLOW;
+				goto pseries_devicetree_update_err;
+			}
 			node = be32_to_cpu(*data++);
 		}
 	} while (rc == 1);
 
+pseries_devicetree_update_err:
 	kfree(rtas_buf);
 	return rc;
 }
-- 
1.9.1