On Tue, Oct 06, 2020 at 04:56:31PM +0200, Patrick Wildt wrote:
> Hi,
>
> I have an EFI application doing TFTP on an i.MX8MM board. The EFI
> application uses the Simple Network Protocol and does ARP itself.
> ARP didn't work, and I saw that the replies that were received on
> the board were broken.
>
> Good, as seen from the sending host:
> 2acbc7b16422001999ed548808060001080006040002001999ed5488ac1f00012acbc7b16422ac1f007f000000000000000000000000000000000000
>
> Bad, as seen on the i.MX8MM board:
> 60a7fc7f0000000060a7fc7f00000000080006040002001999ed5488ac1f00012acbc7b16422ac1f007f000000000000000000000000000000000000
>
> As you can see, in the received packet, it looks like the the first
> 16 bytes were overwritten, and those look like two 64-bit pointer.
>
> Looking through the stack, with uclass enabled, the code does:
>
> eth_rx():
> [..]
> ret = eth_get_ops(current)->recv(current, flags, &packet);
> flags = 0;
> if (ret > 0)
> net_process_received_packet(packet, ret);
> if (ret >= 0 && eth_get_ops(current)->free_pkt)
> eth_get_ops(current)->free_pkt(current, packet, ret);
> [..]
>
> recv returns the packet, free_pkt free()s the packet. Thus we may
> only use the packet's contents between the recv and the free_pkt.
>
> net_process_received_packet():
> [..]
> net_rx_packet = in_packet;
> net_rx_packet_len = len;
> [..]
> if (push_packet) {
> (*push_packet)(in_packet, len);
> return;
> }
> [..]
>
> push_packet is set to efi_net_push during efi_network_timer_notify,
> which does nothing more than to set a status flag:
>
> static void efi_net_push(void *pkt, int len)
> {
> new_rx_packet = true;
> }
>
> This does *not* touch the packet at all. Instead, efi_net_receive
> will, as soon as the EFI application calls it, access net_rx_packet
> directly. But this only happens *after* the packet has already been
> free()d. Something else could have already started using that memory!
>
> The following diff changes efi_net_push() to allocate a new buffer, but
> I think it's not enough since eth_rx() will try to receive 32 packets
> at one time. So I think maybe efi_net_push() needs to add the packets
> to a list, and efi_net_receive() takes the first packet from that list.
>
> Best regards,
> Patrick
This is a better version, since it maintains a list of packets. This
way no packet is missed, since the push method simple pushes a packet
onto the list.
Do we need to purge the list in efi_net_stop() and/or efi_net_reset()?
Signed-off-by: Patrick Wildt <[email protected]>
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 22f0123eca..6381c3898d 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -24,10 +24,28 @@ static const efi_guid_t efi_net_guid =
EFI_SIMPLE_NETWORK_PROTOCOL_GUID;
static const efi_guid_t efi_pxe_base_code_protocol_guid =
EFI_PXE_BASE_CODE_PROTOCOL_GUID;
static struct efi_pxe_packet *dhcp_ack;
-static bool new_rx_packet;
static void *new_tx_packet;
static void *transmit_buffer;
+/*
+ * List of packets that still need to be popped by an application
+ * calling efi_net_receive().
+ */
+LIST_HEAD(efi_packet_queue);
+
+/**
+ * struct efi_net_packet - structure containing packet info
+ *
+ * @link: link to the list of packets to be processed
+ * @pkt: network packet
+ * @len: length
+ */
+struct efi_net_packet {
+ struct list_head link;
+ uchar *pkt;
+ int len;
+};
+
/*
* The notification function of this event is called in every timer cycle
* to check if a new network packet has been received.
@@ -577,6 +595,7 @@ static efi_status_t EFIAPI efi_net_receive
efi_status_t ret = EFI_SUCCESS;
struct ethernet_hdr *eth_hdr;
size_t hdr_size = sizeof(struct ethernet_hdr);
+ struct efi_net_packet *pkt;
u16 protlen;
EFI_ENTRY("%p, %p, %p, %p, %p, %p, %p", this, header_size,
@@ -602,16 +621,18 @@ static efi_status_t EFIAPI efi_net_receive
break;
}
- if (!new_rx_packet) {
+ pkt = list_first_entry_or_null(&efi_packet_queue,
+ struct efi_net_packet, link);
+ if (pkt == NULL) {
ret = EFI_NOT_READY;
goto out;
}
/* Fill export parameters */
- eth_hdr = (struct ethernet_hdr *)net_rx_packet;
+ eth_hdr = (struct ethernet_hdr *)pkt->pkt;
protlen = ntohs(eth_hdr->et_protlen);
if (protlen == 0x8100) {
hdr_size += 4;
- protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
+ protlen = ntohs(*(u16 *)&pkt->pkt[hdr_size - 2]);
}
if (header_size)
*header_size = hdr_size;
@@ -621,17 +642,20 @@ static efi_status_t EFIAPI efi_net_receive
memcpy(src_addr, eth_hdr->et_src, ARP_HLEN);
if (protocol)
*protocol = protlen;
- if (*buffer_size < net_rx_packet_len) {
+ if (*buffer_size < pkt->len) {
/* Packet doesn't fit, try again with bigger buffer */
- *buffer_size = net_rx_packet_len;
+ *buffer_size = pkt->len;
ret = EFI_BUFFER_TOO_SMALL;
goto out;
}
/* Copy packet */
- memcpy(buffer, net_rx_packet, net_rx_packet_len);
- *buffer_size = net_rx_packet_len;
- new_rx_packet = 0;
- this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+ memcpy(buffer, pkt->pkt, pkt->len);
+ *buffer_size = pkt->len;
+ list_del(&pkt->link);
+ free(pkt->pkt);
+ free(pkt);
+ if (list_empty(&efi_packet_queue))
+ this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
out:
return EFI_EXIT(ret);
}
@@ -664,7 +688,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
*/
static void efi_net_push(void *pkt, int len)
{
- new_rx_packet = true;
+ struct efi_net_packet *efi_pkt;
+
+ /* Check that we at least received an Ethernet header */
+ if (len < sizeof(struct ethernet_hdr))
+ return;
+
+ efi_pkt = malloc(sizeof(*efi_pkt));
+ if (efi_pkt == NULL)
+ return;
+
+ efi_pkt->pkt = malloc(len);
+ if (efi_pkt->pkt == NULL) {
+ free(efi_pkt);
+ return;
+ }
+
+ memcpy(efi_pkt->pkt, pkt, len);
+ efi_pkt->len = len;
+
+ list_add_tail(&efi_pkt->link, &efi_packet_queue);
}
/**
@@ -689,20 +732,14 @@ static void EFIAPI efi_network_timer_notify(struct
efi_event *event,
if (!this || this->mode->state != EFI_NETWORK_INITIALIZED)
goto out;
- if (!new_rx_packet) {
+ if (list_empty(&efi_packet_queue)) {
push_packet = efi_net_push;
eth_rx();
push_packet = NULL;
- if (new_rx_packet) {
- /* Check that we at least received an Ethernet header */
- if (net_rx_packet_len >=
- sizeof(struct ethernet_hdr)) {
- this->int_status |=
- EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
- wait_for_packet->is_signaled = true;
- } else {
- new_rx_packet = 0;
- }
+ if (!list_empty(&efi_packet_queue)) {
+ this->int_status |=
+ EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+ wait_for_packet->is_signaled = true;
}
}
out:
