Hi Pedro Falcato,

At 2023-07-31 17:15:20, "Pedro Falcato" <pedro.falcato@gmail.com> wrote:
>On Mon, Jul 31, 2023 at 4:25 AM wangy <wangyzhaoz@163.com> wrote:
>>
>> From: Yang Wang <wangyzhaoz@163.com>
>>
>> The EFI spec (see UEFI 2.10, 24.1.12) requires
>> EFI_SIMPLE_NETWORK.GetStatus() to handle NULL InterruptStatus pointers
>> by not reading nor clearing the interrupt status from the device.
>>
>> However, EmacGetDmaStatus (part of the DwEmacSnpDxe GetStatus()
>> implementation) did not correctly handle NULL IrqStat, despite already
>> being tagged as an OPTIONAL argument. This made calling GetStatus()
>> with a NULL pointer (for example, the call in MnpRecycleTxBuf) either
>> corrupt memory or straight-up crash.
>>
>> Make it EFI spec compliant, by adding proper NULL pointer checks
>> around RI_SET_MSK and TI_SET_MSK retrieval/clearing.
>>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>
>> Signed-off-by: Yang Wang <wangyzhaoz@163.com>
>> Acked-by: Pedro Falcato <pedro.falcato@gmail.com>
>> Reviewed-by: Ran Wang <wangran@bosc.ac.cn>
>> ---
>>  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.c        | 22 ++++++++++++-------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> index 3b982ce984..26d3ff6138 100755
>> --- a/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> +++ b/Silicon/Synopsys/DesignWare/Drivers/DwEmacSnpDxe/EmacDxeUtil.c
>> @@ -500,24 +500,30 @@ EmacGetDmaStatus (
>>    UINT32  ErrorBit;
>>    UINT32  Mask = 0;
>>
>> +  if (IrqStat != NULL) {
>> +    *IrqStat = 0;
>> +  }
>> +
>>    DmaStatus = MmioRead32 (MacBaseAddress +
>>                             DW_EMAC_DMAGRP_STATUS_OFST);
>>    if (DmaStatus & DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK) {
>>      Mask |= DW_EMAC_DMAGRP_STATUS_NIS_SET_MSK;
>>      // Rx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_RI_SET_MSK) {
>> -      *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> -      Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>> +        Mask |= DW_EMAC_DMAGRP_STATUS_RI_SET_MSK;
>> +      }
>>      }
>> +
>>      // Tx interrupt
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TI_SET_MSK) {
>> -      *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> -      Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> -    } else {
>> -      *IrqStat &= ~EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +      if (IrqStat != NULL) {
>> +        *IrqStat |= EFI_SIMPLE_NETWORK_TRANSMIT_INTERRUPT;
>> +        Mask |= DW_EMAC_DMAGRP_STATUS_TI_SET_MSK;
>> +      }
>>      }
>> +
>>      // Tx Buffer
>>      if (DmaStatus & DW_EMAC_DMAGRP_STATUS_TU_SET_MSK){
>>        Mask |= DW_EMAC_DMAGRP_STATUS_TU_SET_MSK;
>> --
>> 2.25.1
>
>Hi,
>
>No need for a v3, Ard has already applied the patch
>(https://github.com/tianocore/edk2-platforms/commit/cbab3c40f76ee913621a9f4afe3398b217b0d086).

Thank you very much, I see.

Regards,
Yang

>
>-- 
>Pedro