[Barebox] Polling infrastructure causes reentrancy problem

* [Barebox] Polling infrastructure causes reentrancy problem
@ 2021-11-05 12:40 Christian Eggers
  2021-11-08 10:34 ` Sascha Hauer
  0 siblings, 1 reply; 3+ messages in thread
From: Christian Eggers @ 2021-11-05 12:40 UTC (permalink / raw)
  To: Ahmad Fatoum, Sascha Hauer, Marc Kleine-Budde; +Cc: barebox

Disclaimer: I stumbled over this on barebox 2020.01.
I haven't tried to reproduce this on the current version.

Summary:
----------
Use case: Linux kernel is started via Android fastboot (uses polling infrastructure) while probing of other device drivers is ongoing.
Behavior: usb_stor_disconnect() is called before usb_stor_probe() returned
Result: NULL pointer dereference in usb_stor_disconnect()

Details:
----------
I use fastboot over USB gadget for loading a FIT image (I have
stripped down/modified the Android fastboot command for this).
The fastboot command (on the PC) is executed via udev, so the
transfer of the FIT image starts as soon as fastboot has been
activated on the barebox side.

"Parallel" to the fastboot transfer, my (custom) barebox_main
scans for USB devices (e.g. for alternative recovery option via
USB thumb driver). During detection/registration of the first
USB drive, there are delay() statements which causes progress
on the fastboot file transfer. As soon as the FIT transfer has
finished, do_bootm_linux/shutdown_barebox is called. Here the
USB storage device is disconnected without ever having been
fully probed.

Call stack (start reading from bottom):
----------
-000|usb_stor_disconnect(usbdev = 0x9010D0B0)
--> usb_stor_disconnect is called with a partially probed device
--> NULL pointer dereference 

/* Handle a USB mass-storage disconnect */
static void usb_stor_disconnect(struct usb_device *usbdev)
{
	struct us_data *us = (struct us_data *)usbdev->drv_data;
--> us = NULL!
	struct us_blk_dev *bdev, *bdev_tmp;

...

-001|devices_shutdown()
-002|shutdown_barebox()
-003|start_linux(adr = 0x82000000, swap = 0, initrd_address = ?, initrd_size = 5739640, oftree = 0x828A20
-004|__do_bootm_linux(:data = 0x915FF560, free_mem = 2190090240, swap = 0, fdt = ?)
-005|do_bootm_linux(:data = 0x915FF560)
-006|bootm_boot(:bootm_data = 0x9FFEFBF8)
-007|cb_boot(:f_fb = 0x8FE644B0, opt = ?)
-008|fb_run_command(inline)
--> fastboot transfer is finished
-008|rx_handler_command(:ep = 0x8FE53050, :req = 0x8FE645F8)
-009|list_del_init(inline)
-009|done(:ep = 0x8FE53050, :req = 0x8FE645F8, status = ?)
-010|dtd_complete_irq(inline)
-010|fsl_udc_gadget_poll(inline)
-010|fsl_udc_gadget_poll(gadget = 0x8FE52EA0)
-011|poller_call()
-012|is_timeout(:start_ns = 34359738368, :time_offset_ns = 0)
-013|udelay(:usecs = 100)
--> delay caues progress on fastboot transfer
-013|mdelay(tailcall)
-014|usb_stor_transport(usb_blkdev = 0x9038DA48, cmd = 0x9FFEFD40 -> "", cmdlen = ?, data = 0x0, datalen
-015|usb_stor_test_unit_ready(:usb_blkdev = 0x9038DA48)
-016|usb_stor_init_blkdev(inline)
-016|usb_stor_add_blkdev(inline)
-016|usb_stor_scan(inline)
-016|usb_stor_probe(:usbdev = 0x9010DBC0, id = ?)
--> usb_store_probe() starts registration of USB device
-017|list_add(inline)
-017|device_probe(:dev = 0x9010DBC0)
-018|match(:drv = 0x9FC4B11C, :dev = 0x9010DBC0)
-019|list_add_tail(inline)
-019|register_device(:new_device = 0x9010DBC0)
-020|usb_new_device(:dev = 0x9010D0B0)
-021|usb_hub_port_connect_change(:dev = 0x8FE69A30, :port = 4)
-022|usb_scan_port(inline)
-022|usb_device_list_scan(inline)
-022|usb_hub_configure_ports(inline)
-022|usb_hub_detect(dev = 0x8FE692A0)
-023|usb_host_detect(:host = 0x8FE558A0)
-024|usb_rescan()
-025|orbiter_usb_recovery(inline)
-025|orbiter_recovery(inline)
--> Fastboot is activated somewhere here.
-025|orbiter_recovery()
-026|start_barebox()
-027|barebox_non_pbl_start(membase = 2147483648, memsize = ?, boarddata = 0x9FC3A6BF)
-028|start(membase = ?, memsize = ?, boarddata = ?)
 ---|end of frame

Probably I shouldn't use fastboot this way (causing linux boot from polling
infrastructure). On the other hand, using fastboot allows taking control from
a PC without the need for manually entering commands in barebox (which is
quite useful in my application).

As a first step, I will try to check for NULL pointer in usb_stor_disconnect().
But I guess there should be a more generic solution (e.g. not calling
disconnect/remove for partially probed devices).

regards
Christian

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 3+ messages in thread