* [PATCH 1/2] powerpc/pseries: fix endian bugs in mobility RTAS calls
2014-09-16 0:25 [PATCH 0/2] powerpc/pseries: RTAS mobility fixes Cyril Bur
@ 2014-09-16 0:25 ` Cyril Bur
2014-09-16 0:25 ` [PATCH 2/2] powerpc/pseries: fix bugs in RTAS mobility code Cyril Bur
1 sibling, 0 replies; 3+ messages in thread
From: Cyril Bur @ 2014-09-16 0:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH 2/2] powerpc/pseries: fix bugs in RTAS mobility code
2014-09-16 0:25 [PATCH 0/2] powerpc/pseries: RTAS mobility fixes Cyril Bur
2014-09-16 0:25 ` [PATCH 1/2] powerpc/pseries: fix endian bugs in mobility RTAS calls Cyril Bur
@ 2014-09-16 0:25 ` Cyril Bur
1 sibling, 0 replies; 3+ messages in thread
From: Cyril Bur @ 2014-09-16 0:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: 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
^ permalink raw reply related [flat|nested] 3+ messages in thread