Re: [Linaro-uefi] [PATCH] OpenPlatformPkg/Lan91xDxe: Fixed driver crash and TX recycle buffer

Hi Daniil,

I think the contents of this patch are generally OK, but there are a few issues.

On 7 June 2016 at 01:25, Daniil Egranov daniil.egranov@arm.com wrote:

Fixed driver crash on ARP packets. Fixed TX recycle buffer overflow. Added prototype for SnpWaitForPacketNotify SNP call and notify event.

It seems that this patch is doing three separate things, so I'd expect three patches.

Can you explain how you reproduce the crash and buffer overflow and explain the purpose of the SNP event?

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

Drivers/Net/Lan91xDxe/Lan91xDxe.c | 179 +++++++++++++++++++++++++------------- 1 file changed, 118 insertions(+), 61 deletions(-)

diff --git a/Drivers/Net/Lan91xDxe/Lan91xDxe.c b/Drivers/Net/Lan91xDxe/Lan91xDxe.c index 3f857a5..45b330f 100644 --- a/Drivers/Net/Lan91xDxe/Lan91xDxe.c +++ b/Drivers/Net/Lan91xDxe/Lan91xDxe.c @@ -65,10 +65,7 @@ typedef struct _LAN91X_DRIVER { EFI_NETWORK_STATISTICS Stats;

// Transmit Buffer recycle queue -#define TX_QUEUE_DEPTH 16

• VOID *TxQueue[TX_QUEUE_DEPTH];
• UINTN TxQueHead;
• UINTN TxQueTail;

• LIST_ENTRY TransmitQueueHead;

  // Register access variables UINTN IoBase; // I/O Base Address

@@ -137,47 +134,18 @@ STATIC CHAR16 CONST * CONST ChipIds[ 16 ] = { NULL, NULL, NULL };

+typedef struct {

• VOID *Buf;
• UINTN Length;

+} MSK_SYSTEM_BUF;

-/* ------------------ TxBuffer Queue functions ------------------- */

-#define TxQueNext(off) ((((off) + 1) >= TX_QUEUE_DEPTH) ? 0 : ((off) + 1))

-STATIC -BOOLEAN -TxQueInsert (

• IN LAN91X_DRIVER *LanDriver,
• IN VOID *Buffer
• )

-{

• if (TxQueNext (LanDriver->TxQueTail) == LanDriver->TxQueHead) {
• return FALSE;
• }
• LanDriver->TxQueue[LanDriver->TxQueTail] = Buffer;
• LanDriver->TxQueTail = TxQueNext (LanDriver->TxQueTail);
• return TRUE;

-}

-STATIC -VOID -*TxQueRemove (

• IN LAN91X_DRIVER *LanDriver
• )

-{

• VOID *Buffer;
• if (LanDriver->TxQueTail == LanDriver->TxQueHead) {
• return NULL;
• }
• Buffer = LanDriver->TxQueue[LanDriver->TxQueHead];
• LanDriver->TxQueue[LanDriver->TxQueHead] = NULL;
• LanDriver->TxQueHead = TxQueNext (LanDriver->TxQueHead);

+typedef struct {

• UINTN Signature;
• LIST_ENTRY Link;
• MSK_SYSTEM_BUF SystemBuf;

+} MSK_LINKED_SYSTEM_BUF;

• return Buffer;

-} +#define TX_MBUF_SIGNATURE SIGNATURE_32 ('t','x','m','b')

/* ------------------ MAC Address Hash Calculations ------------------- */

@@ -1105,6 +1073,42 @@ exit_unlock: return Status; }

+/**

• Notification call back function for WaitForPacket event.
• <at> param Event EFI Event.
• <at> param SnpPtr Pointer to YUKON_DRIVER structure.

+**/ +VOID +EFIAPI +SnpWaitForPacketNotify (

• EFI_EVENT Event,
• VOID *SnpPtr
• )

+{

• DEBUG ((EFI_D_NET, "LAN91x: SnpWaitForPacketNotify()\n"));
• //
• // Do nothing if either parameter is a NULL pointer.
• //
• if (Event == NULL || SnpPtr == NULL) {
• return ;
• }
• //
• // Do nothing if the SNP interface is not initialized.
• //
• switch (((LAN91X_DRIVER *) SnpPtr)->SnpMode.State) {
• case EfiSimpleNetworkInitialized:

•  break;
  

• case EfiSimpleNetworkStopped:
• case EfiSimpleNetworkStarted:
• default:

•  return ;
  

• }

+}

This function does nothing, it just returns. Is it supposed to be doing something? If not, but yet it's needed, why doesn't it just return? No parameter checking or state checking is needed. Explaining the purpose of the event seems more important when it seems that it does nothing.

/* ** UEFI Initialize() function ** @@ -1148,10 +1152,25 @@ SnpInitialize ( // Find the LanDriver structure LanDriver = INSTANCE_FROM_SNP_THIS(Snp);

• Status = gBS->CreateEvent (

•    EVT_NOTIFY_WAIT,
  

•    TPL_NOTIFY,
  

•    &SnpWaitForPacketNotify,
  

•    LanDriver,
  

•    &LanDriver->Snp.WaitForPacket
  

•    );
  

• if (EFI_ERROR (Status)) {
• LanDriver->Snp.WaitForPacket = NULL;
• Status = EFI_DEVICE_ERROR;
• ReturnUnlock (EFI_DEVICE_ERROR);
• }
• // Initiate a software reset Status = SoftReset (LanDriver); if (EFI_ERROR(Status)) { DEBUG((EFI_D_WARN, "LAN91x: Soft reset failed\n"));
• gBS->CloseEvent (LanDriver->Snp.WaitForPacket); ReturnUnlock (EFI_DEVICE_ERROR); }

@@ -1159,6 +1178,7 @@ SnpInitialize ( if (PhySoftReset (LanDriver) < 0) { Snp->Mode->State = EfiSimpleNetworkStopped; DEBUG((EFI_D_WARN, "LAN91x: PHY soft reset timeout\n"));

• gBS->CloseEvent (LanDriver->Snp.WaitForPacket); ReturnUnlock (EFI_NOT_STARTED); }

@@ -1643,11 +1663,12 @@ SnpGetStatus ( OUT VOID **TxBuff OPTIONAL ) {

• LAN91X_DRIVER *LanDriver;
• EFI_TPL SavedTpl;
• EFI_STATUS Status;
• BOOLEAN MediaPresent;
• UINT8 IstReg;

• LAN91X_DRIVER *LanDriver;
• EFI_TPL SavedTpl;
• EFI_STATUS Status;
• BOOLEAN MediaPresent;
• UINT8 IstReg;
• MSK_LINKED_SYSTEM_BUF *m_head;

That variable name is not in keeping with the style in this file. How about something like LinkedSystemBuf (as used elsewhere)?

// Check preliminaries if (Snp == NULL) { @@ -1689,8 +1710,21 @@ SnpGetStatus ( }

// Pass back the completed buffer address

• // The transmit buffer status is not read when TxBuf is NULL if (TxBuff != NULL) {

• *TxBuff = TxQueRemove (LanDriver);

• *((UINT8 **) TxBuff) = (UINT8 *) 0;
• if( !IsListEmpty (&LanDriver->TransmitQueueHead))
• {

•  m_head = CR (GetFirstNode (&LanDriver->TransmitQueueHead), MSK_LINKED_SYSTEM_BUF, Link, TX_MBUF_SIGNATURE);
  

•  if(m_head != NULL) {
  

•    *TxBuff = m_head->SystemBuf.Buf;
  

•    RemoveEntryList (&m_head->Link);
  

•    FreePool (m_head);
  

Looking at this code, at first I thought you were freeing the entire linked list, not just the first element. So I think a rename of m_head would indeed be beneficial here.

•  }
  

• }
• // if (TxBuff != NULL) {
• // *TxBuff = TxQueRemove (LanDriver);

You shouldn't add commented out code.

}

// Update the media status @@ -1733,6 +1767,8 @@ SnpTransmit ( UINTN Retries; UINT16 Proto; UINT8 PktNum;

• MSK_LINKED_SYSTEM_BUF *LinkedSystemBuf;

• // Check preliminaries if ((Snp == NULL) || (BufAddr == NULL)) {

@@ -1818,12 +1854,18 @@ SnpTransmit ( PktNum &= ARR_PACKET;

// Check for the nature of the frame

• if (DstAddr->Addr[0] == 0xFF) {
• LanDriver->Stats.TxBroadcastFrames += 1;
• } else if ((DstAddr->Addr[0] & 0x1) == 1) {
• LanDriver->Stats.TxMulticastFrames += 1;

• // If no destination address, it's ARP broadcast
• if(DstAddr != NULL)
• {
• if (DstAddr->Addr[0] == 0xFF) {

•  LanDriver->Stats.TxBroadcastFrames += 1;
  

• } else if ((DstAddr->Addr[0] & 0x1) == 1) {

•  LanDriver->Stats.TxMulticastFrames += 1;
  

• } else {

•  LanDriver->Stats.TxUnicastFrames += 1;
  

• } } else {

• LanDriver->Stats.TxUnicastFrames += 1;

• LanDriver->Stats.TxBroadcastFrames += 1; }

  // Set the Packet Number and Pointer registers

@@ -1877,14 +1919,27 @@ SnpTransmit ( ReturnUnlock (EFI_DEVICE_ERROR); }

• // Update the Rx statistics

• // Update the Tx statistics LanDriver->Stats.TxTotalBytes += BufSize; LanDriver->Stats.TxGoodFrames += 1;

  // Update the Tx Buffer cache

• if (!TxQueInsert (LanDriver, BufAddr)) {
• DEBUG((EFI_D_WARN, "LAN91x: SnpTransmit(): TxQueue insert failure.\n"));
• }

• LinkedSystemBuf = AllocateZeroPool (sizeof (MSK_LINKED_SYSTEM_BUF));

• if (LinkedSystemBuf == NULL) {

• return EFI_OUT_OF_RESOURCES;

• }

• LinkedSystemBuf->Signature = TX_MBUF_SIGNATURE;

• //

• // Add the passed Buffer to the transmit queue. Don't copy.

• //

• LinkedSystemBuf->SystemBuf.Buf = BufAddr;

• LinkedSystemBuf->SystemBuf.Length = BufSize;

• InsertTailList (&LanDriver->TransmitQueueHead, &LinkedSystemBuf->Link);

• // if (!TxQueInsert (LanDriver, BufAddr)) {

• // DEBUG((EFI_D_WARN, "LAN91x: SnpTransmit(): TxQueue insert failure.\n"));

• // }

• Status = EFI_SUCCESS;

  // Dump the packet header

@@ -1941,7 +1996,7 @@ SnpReceive ( // Serialize access to data and registers SavedTpl = gBS->RaiseTPL (LAN91X_TPL);

• // Check that driver was started and initialised

• // Check that driver was started and initialized

Changing spelling from English to US English seems a bit unnecessary :-O The file is already littered with both variants. Maybe you should create a separate patch to make them all consistent if it bothers you?

switch (Snp->Mode->State) { case EfiSimpleNetworkInitialized: break; @@ -2104,7 +2159,6 @@ exit_unlock: return Status; }

/*------------------ Driver Execution Environment main entry point ------------------*/

/* @@ -2155,9 +2209,12 @@ Lan91xDxeEntry ( PrintPhyRegisters (LanDriver); #endif

• // Initialize transmit queue
• InitializeListHead (&LanDriver->TransmitQueueHead);
• // Assign fields and func pointers Snp->Revision = EFI_SIMPLE_NETWORK_PROTOCOL_REVISION;

• Snp->WaitForPacket = NULL;

• Snp->WaitForPacket = SnpWaitForPacketNotify; Snp->Initialize = SnpInitialize; Snp->Start = SnpStart; Snp->Stop = SnpStop;

-- 2.7.4

---

Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi