Re: U-Boot interferes with initrd binary — Revert "efi: Correct smbios-table installation" ?

[neil.armstrong at linaro.org]

Hi Christian

On 04/04/2025 00:41, Simon Glass wrote:
> Hi Christian,
> 
> On Fri, 4 Apr 2025 at 08:18, Christian Kohlschütter
> <christian at kohlschutter.com> wrote:
>>
>> +Tom Rini (actually adding Tom to the conversation)
>>
>>> On 3. Apr 2025, at 19:54, Simon Glass <sjg at chromium.org> wrote:
>>>
>>> +Tom Rini in case this affects the release
>>>
>>> Hi Christian,
>>>
>>> On Fri, 4 Apr 2025 at 01:46, Christian Kohlschütter
>>> <christian at kohlschutter.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>>> On 1. Apr 2025, at 17:51, Simon Glass <sjg at 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