Re: [edk2-devel] [edk2-platforms][PATCH V3] Silicon/Synopsys/DesignWare: DwEmacSnpDxe: Fix bug in EmacGetDmaStatus

["Pedro Falcato" <pedro.falcato@gmail.com>]

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).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107398): https://edk2.groups.io/g/devel/message/107398
Mute This Topic: https://groups.io/mt/100455239/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-