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@xxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Frank Rowand <frowand.list@xxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxx>
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
