AnsweredAssumed Answered

Bug in the LwIP ethernet driver

Question asked by Deomid Ryabkov on Oct 24, 2016
Latest reply on Oct 24, 2016 by manfredschnell

ethernetif.c has this code to deal with an incoming frame:


        /* Call ENET_ReadFrame when there is a received frame. */
        if (length != 0)
            uint8_t *receivedDataBuffer;
            struct pbuf *packetBuffer;
            packetBuffer = pbuf_alloc(PBUF_RAW, length, PBUF_POOL);
            /* Received valid frame. Deliver the rx buffer with the size equal to length. */
            if ((packetBuffer == NULL) || (packetBuffer->payload == NULL))
                LWIP_ASSERT("Fail to allocate new memory space", 0);
                /* Should not reach this statement if error occurs */
            receivedDataBuffer = packetBuffer->payload;
            packetBuffer->len = (u16_t)length;
            ENET_ReadFrame(ENET, handle, receivedDataBuffer, packetBuffer->len);


the problem with this code is that it assumes that all length bytes will be allocated in one contiguous buffer, which may not be the case: pbuf_alloc(PBUF_POOL) can return a chain of pbufs adding up to the requested length.

in that case packetBuffer->len is not the same as length - instead, length of the entire chain is held in packetBuffer->tot_len


to illustrate, a concrete example i've just spent considerable time debugging: 704 bytes are allocated as a chain of 2 pbufs, 636 and 68 bytes. packetBuffer points at the first buffer, packetBuffer->len is 636, packetBuffer->tot_len is 704 and packetBuffer->next is non-NULL, pointing to the second 64 pbuf. however, at line 15 packetBuffer->len is rather unceremoniously stomped with 704 and we then proceed to call ENET_ReadFrame which will then copy 704 bytes to packetBuffer->payload, but which is not large enough to receive that amount - its size is 636 bytes... BANG! memory is stomped, all sorts of hell breaks loose and nuclear missiles are launched (maybe). 


it looks like whoever was writing this code is not familiar with lwip's pbuf chains, as evidenced by stomping of pbuf->len - that should never be done!