[PATCH] of/fdt: avoid undefined behaviour in populate_properties()

[PATCH] of/fdt: avoid undefined behaviour in populate_properties()

[Mark Rutland]

We unflatten a device tree in two passes: the first calculating the size
of the unflattened tree, and the second performing the actual
unflattening into a suitably-sized buffer.

During the first (dryrun) pass, the memory pool is NULL, and we derive
other pointers from this. Mostly these are done though intermediate
casts to unsigned long, which prevents the compiler from being able to
observe this as undefined behaviour. However, in populate_properties()
we derive the pprev pointer from the np pointer, which is NULL if it is
the first element allocated from the memory pool.

This is detected by UBSAN:

================================================================================
UBSAN: Undefined behaviour in drivers/of/fdt.c:190:8
member access within null pointer of type 'struct device_node'
CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc3+ #13
Hardware name: ARM Juno development board (r1) (DT)
Call trace:
 dump_backtrace+0x0/0x458
 show_stack+0x20/0x30
 dump_stack+0x18c/0x248
 ubsan_epilogue+0x18/0x94
 handle_null_ptr_deref+0x1d4/0x228
 __ubsan_handle_type_mismatch_v1+0x188/0x1e0
 unflatten_dt_nodes+0xd40/0x1000
 __unflatten_device_tree+0x58/0x248
 unflatten_device_tree+0x38/0x4c
 setup_arch+0x3b0/0xcc4
 start_kernel+0xd8/0xb9c
================================================================================

In the dryrun pass we don't actually use the pprev value, so let's only
set it when !dryrun, and avoid the undefined behaviour.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org
---
 drivers/of/fdt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6da20b9688f7..c1d0c348f2b3 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -187,7 +187,9 @@ static void populate_properties(const void *blob,
 	int cur;
 	bool has_name = false;
 
-	pprev = &np->properties;
+	if (!dryrun)
+		pprev = &np->properties;
+
 	for (cur = fdt_first_property_offset(blob, offset);
 	     cur >= 0;
 	     cur = fdt_next_property_offset(blob, cur)) {
-- 
2.11.0

Re: [PATCH] of/fdt: avoid undefined behaviour in populate_properties()

[Frank Rowand]

On 07/06/18 04:37, Mark Rutland wrote:
> We unflatten a device tree in two passes: the first calculating the size
> of the unflattened tree, and the second performing the actual
> unflattening into a suitably-sized buffer.
> 
> During the first (dryrun) pass, the memory pool is NULL, and we derive
> other pointers from this. Mostly these are done though intermediate
> casts to unsigned long, which prevents the compiler from being able to
> observe this as undefined behaviour. However, in populate_properties()
> we derive the pprev pointer from the np pointer, which is NULL if it is
> the first element allocated from the memory pool.
> 
> This is detected by UBSAN:
> 
> ================================================================================
> UBSAN: Undefined behaviour in drivers/of/fdt.c:190:8
> member access within null pointer of type 'struct device_node'
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.18.0-rc3+ #13
> Hardware name: ARM Juno development board (r1) (DT)
> Call trace:
>  dump_backtrace+0x0/0x458
>  show_stack+0x20/0x30
>  dump_stack+0x18c/0x248
>  ubsan_epilogue+0x18/0x94
>  handle_null_ptr_deref+0x1d4/0x228
>  __ubsan_handle_type_mismatch_v1+0x188/0x1e0
>  unflatten_dt_nodes+0xd40/0x1000
>  __unflatten_device_tree+0x58/0x248
>  unflatten_device_tree+0x38/0x4c
>  setup_arch+0x3b0/0xcc4
>  start_kernel+0xd8/0xb9c
> ================================================================================
> 
> In the dryrun pass we don't actually use the pprev value, so let's only
> set it when !dryrun, and avoid the undefined behaviour.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: devicetree@vger.kernel.org
> ---
>  drivers/of/fdt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 6da20b9688f7..c1d0c348f2b3 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -187,7 +187,9 @@ static void populate_properties(const void *blob,
>  	int cur;
>  	bool has_name = false;
>  
> -	pprev = &np->properties;
> +	if (!dryrun)
> +		pprev = &np->properties;
> +
>  	for (cur = fdt_first_property_offset(blob, offset);
>  	     cur >= 0;
>  	     cur = fdt_next_property_offset(blob, cur)) {
> 

Please add a one line comment explaining why "if (!dryrun)" so that no one
decides to remove the test in the future as not needed.  Otherwise,

Reviewed-by: Frank Rowand <frank.rowand@sony.com>

-Frank

Re: [PATCH] of/fdt: avoid undefined behaviour in populate_properties()

[Rob Herring]

On Fri, Jul 6, 2018 at 10:51 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 07/06/18 04:37, Mark Rutland wrote:
> > We unflatten a device tree in two passes: the first calculating the size
> > of the unflattened tree, and the second performing the actual
> > unflattening into a suitably-sized buffer.

[...]

> > @@ -187,7 +187,9 @@ static void populate_properties(const void *blob,
> >       int cur;
> >       bool has_name = false;
> >
> > -     pprev = &np->properties;
> > +     if (!dryrun)
> > +             pprev = &np->properties;
> > +
> >       for (cur = fdt_first_property_offset(blob, offset);
> >            cur >= 0;
> >            cur = fdt_next_property_offset(blob, cur)) {
> >
>
> Please add a one line comment explaining why "if (!dryrun)" so that no one
> decides to remove the test in the future as not needed.  Otherwise,

Please re-spin ASAP with the comment if you'd like this in 4.18.

Rob