Hi Christian
On 04/04/2025 00:41, Simon Glass wrote:
Hi Christian,
On Fri, 4 Apr 2025 at 08:18, Christian Kohlschütter
<christ...@kohlschutter.com> wrote:
+Tom Rini (actually adding Tom to the conversation)
On 3. Apr 2025, at 19:54, Simon Glass <s...@chromium.org> wrote:
+Tom Rini in case this affects the release
Hi Christian,
On Fri, 4 Apr 2025 at 01:46, Christian Kohlschütter
<christ...@kohlschutter.com> wrote:
Hi Simon,
On 1. Apr 2025, at 17:51, Simon Glass <s...@chromium.org> wrote:
but I don't know precisely what these various functions are supposed to
do, and I can't find any path that leads from any of these to eth_halt().
Is it possible that U-Boot is failing to call eth_halt() in response to
ExitBootServices(), and is therefore leaving the network device active
and performing DMA while the kernel starts up?
The dm_remove_devices_active() is supposed to handle this, but it is
possible that one of the drivers lacks a remove() method.
for what it's worth, this is happening on both ODROID N2+ (meson8b-dwmac) as
well as NanoPI R4S (rk_gmac-dwmac).
I also don't understand why reverting 06ef8089f876b6dabf56caba31a05c003f03c629
"fixes" this behavior. Does it mean that any memalign / malloc may cause this?
Also, how can this failure mode be detected and prevented in code?
I really don't fancy the thought of remote code injection into Linux initrd by
spoofing Ethernet packages, but it really looks like a possibility.
Here's my idea of what might be going on based on Michael's observations:
1. designware_eth_start() starts the interface and
designware_eth_stop() stops it
2. tx_descs_init() sets up the DMA and uses the device-private info
(struct dw_eth_dev)
3. When the device is removed, the struct is freed, meaning that a
future malloc() can use that same space.
Yes, that sounds plausible. How can such allocations be prevented?
There is no need to prevent allocations. It is in the nature of
malloc() that any memory you free can then be used by a subsequent
malloc(). The bug here is that DMA needs to be disabled when the
device is removed.
I assume U-Boot's malloc does not know anything about iPXE's or Linux's
allocation, and vice versa?
4. DMA traffic could then write over the malloc() region
I'm not seeing where the Ethernet device's stop() is called. The
dwmac_meson8b driver does not have a remove() method, so presumably
DMA is still running after the device is removed. Probably the correct
fix would be to add a remove() method to that driver.
Right. Of course this means that there's still a chance that some future driver
would again fail to do this.
How can we prevent this? Can some removal hook be added automatically upon
registration?
Visual inspection of each network driver should help.
I'm not sure if we want to 'stop' all the network devices before
booting Linux, but perhaps we could perhaps provide that feature as a
Kconfig option, e.g. in announce_and_cleanup(), which could really use
a cleanup to make it common across archs.
could you test:
==========><==========================
diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
index fde4aabbace..bf9c5e8f349 100644
--- a/drivers/net/dwmac_meson8b.c
+++ b/drivers/net/dwmac_meson8b.c
@@ -145,6 +145,15 @@ static int dwmac_meson8b_probe(struct udevice *dev)
return designware_eth_probe(dev);
}
+static int dwmac_meson8b_remove(struct udevice *dev)
+{
+ struct dwmac_meson8b_plat *pdata = dev_get_plat(dev);
+
+ designware_eth_stop(dev);
+
+ return designware_eth_remove(dev);
+}
+
static const struct udevice_id dwmac_meson8b_ids[] = {
{ .compatible = "amlogic,meson-gxbb-dwmac", .data =
(ulong)dwmac_setup_gx },
{ .compatible = "amlogic,meson-g12a-dwmac", .data =
(ulong)dwmac_setup_axg },
@@ -158,6 +167,7 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
.of_match = dwmac_meson8b_ids,
.of_to_plat = dwmac_meson8b_of_to_plat,
.probe = dwmac_meson8b_probe,
+ .remove = dwmac_meson8b_remove,
.ops = &designware_eth_ops,
.priv_auto = sizeof(struct dw_eth_dev),
.plat_auto = sizeof(struct dwmac_meson8b_plat),
==========><==========================
Thanks,
Neil
Regards,
Simon
Thanks for looking into this!
Thanks for the report!
Christian
Regards,
Simon
