From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kKHMv-0001eX-Ni for barebox@lists.infradead.org; Mon, 21 Sep 2020 08:40:18 +0000 Date: Mon, 21 Sep 2020 10:40:16 +0200 From: Sascha Hauer Message-ID: <20200921084016.GG21278@pengutronix.de> References: <20200916135035.7089-1-a.fatoum@pengutronix.de> <20200916135035.7089-5-a.fatoum@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200916135035.7089-5-a.fatoum@pengutronix.de> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 04/10] power: reset: reboot-mode: fix up node into boot device tree To: Ahmad Fatoum Cc: barebox@lists.infradead.org On Wed, Sep 16, 2020 at 03:50:29PM +0200, Ahmad Fatoum wrote: > Instead of relying that the kernel and barebox device trees are in sync, > just enforce it by having barebox fix up the device tree node it probed > into the kernel device tree. We usually want that, but some reboot mode > drivers might want to inhibit the fixup, e.g. because they implement > a non-upstream binding or because they communicate with the BootROM, > while the kernel shouldn't. For those the fixup is made optional via > a struct reboot_mode_driver::no_fixup member. > > Signed-off-by: Ahmad Fatoum > --- > drivers/power/reset/reboot-mode.c | 39 +++++++++++++++++++++++++++++++ > include/linux/reboot-mode.h | 1 + > 2 files changed, 40 insertions(+) > > diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c > index 5992a2acd99a..5fc29ebf8091 100644 > --- a/drivers/power/reset/reboot-mode.c > +++ b/drivers/power/reset/reboot-mode.c > @@ -52,6 +52,42 @@ static int reboot_mode_add_param(struct device_d *dev, > return PTR_ERR_OR_ZERO(param); > } > > +static struct device_node *of_get_node_by_reproducible_name(struct device_node *dstroot, > + struct device_node *srcnp) > +{ > + struct device_node *dstnp; > + char *name; > + > + name = of_get_reproducible_name(srcnp); > + dstnp = of_find_node_by_reproducible_name(dstroot, name); > + free(name); > + > + return dstnp; > +} > + > +static int of_reboot_mode_fixup(struct device_node *root, void *ctx) > +{ > + struct reboot_mode_driver *reboot = ctx; > + struct device_node *dstnp, *srcnp, *dstparent; > + > + srcnp = reboot->dev->device_node; > + > + dstnp = of_get_node_by_reproducible_name(root, srcnp); > + if (dstnp) { > + dstparent = dstnp->parent; > + of_delete_node(dstnp); > + } else { > + dstparent = of_get_node_by_reproducible_name(root, srcnp->parent); > + } \o/ barebox KASan found it's first bug! This ends up in: ERROR: ================================================================== ERROR: BUG: KASAN: out-of-bounds in of_copy_node+0x11/0x8e ERROR: Read of size 4 at addr 2fe0a2a0 ERROR: WARNING: [<4fcd3101>] (unwind_backtrace+0x1/0x100) from [<4fc93e0d>] (kasan_report+0xa9/0x214) WARNING: [<4fc93e0d>] (kasan_report+0xa9/0x214) from [<4fc7cd83>] (of_copy_node+0x11/0x8e) WARNING: [<4fc7cd83>] (of_copy_node+0x11/0x8e) from [<4fc811b7>] (of_reboot_mode_fixup+0x9f/0xd4) WARNING: [<4fc811b7>] (of_reboot_mode_fixup+0x9f/0xd4) from [<4fc0bda9>] (of_fix_tree+0x3d/0x70) WARNING: [<4fc0bda9>] (of_fix_tree+0x3d/0x70) from [<4fc0bdf5>] (of_get_fixed_tree+0x19/0x2c) WARNING: [<4fc0bdf5>] (of_get_fixed_tree+0x19/0x2c) from [<4fc05ae9>] (bootm_get_devicetree+0x279/0x2b4) WARNING: [<4fc05ae9>] (bootm_get_devicetree+0x279/0x2b4) from [<4fcd2111>] (__do_bootm_linux+0xdd/0x1e0) WARNING: [<4fcd2111>] (__do_bootm_linux+0xdd/0x1e0) from [<4fcd2299>] (do_bootm_linux+0x85/0xa0) WARNING: [<4fcd2299>] (do_bootm_linux+0x85/0xa0) from [<4fc062bd>] (bootm_boot+0x60d/0x684) WARNING: [<4fc062bd>] (bootm_boot+0x60d/0x684) from [<4fc820d5>] (do_bootm+0x1b5/0x1f8) WARNING: [<4fc820d5>] (do_bootm+0x1b5/0x1f8) from [<4fc06dd3>] (execute_command+0x6b/0xb4) WARNING: [<4fc06dd3>] (execute_command+0x6b/0xb4) from [<4fc0f835>] (run_list_real+0x985/0x9f0) WARNING: [<4fc0f835>] (run_list_real+0x985/0x9f0) from [<4fc0ed05>] (parse_stream_outer+0x221/0x2a8) WARNING: [<4fc0ed05>] (parse_stream_outer+0x221/0x2a8) from [<4fc0fc0b>] (run_shell+0x8b/0xdc) WARNING: [<4fc0fc0b>] (run_shell+0x8b/0xdc) from [<4fc01895>] (run_init+0x205/0x264) WARNING: [<4fc01895>] (run_init+0x205/0x264) from [<4fc01933>] (start_barebox+0x3f/0xa4) WARNING: [<4fc01933>] (start_barebox+0x3f/0xa4) from [<4fcd09f9>] (barebox_non_pbl_start+0x131/0x174) WARNING: [<4fcd09f9>] (barebox_non_pbl_start+0x131/0x174) from [<4fc00005>] (__bare_init_start+0x1/0xc) This happens in the case when the to-be-fixed up device tree is the internal device tree itself. Here of_delete_node(dstnp) is called, but this is also srcnp which is then read from. The solution might also be to make sure we make a copy of the internal tree before doing a of_fixup on it. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox