[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 at 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