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....@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
