efi_loader: fix free() before use in RX path

[Patrick Wildt <patrick@blueri.se>]

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 <patrick@blueri.se>

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: