[PATCH v2 2/2] efi_loader: fix use after free in receive path

Patrick Wildt patrick at blueri.se
Wed Oct 7 00:59:01 CEST 2020


With DM enabled the ethernet code will receive a packet, call
the push method that's set by the EFI network implementation
and then free the packet.  Unfortunately the push methods only
sets a flag that the packet needs to be handled, but the code
that provides the packet to an EFI application runs after the
packet has already been freed.

To rectify this issue, adjust the push method to accept the packet
and store it in a temporary buffer.  The EFI application then gets
the data copied from that buffer.  This way the packet is cached
until is is needed.

The DM Ethernet stack tries to receive 32 packets at once, thus
we better allocate as many buffers as the stack.

Signed-off-by: Patrick Wildt <patrick at blueri.se>
---
 lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 22f0123eca..ed72df42b8 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -24,9 +24,12 @@ 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;
+static uchar **receive_buffer;
+static size_t *receive_lengths;
+static int rx_packet_idx;
+static int rx_packet_num;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -602,16 +605,16 @@ static efi_status_t EFIAPI efi_net_receive
 		break;
 	}
 
-	if (!new_rx_packet) {
+	if (!rx_packet_num) {
 		ret = EFI_NOT_READY;
 		goto out;
 	}
 	/* Fill export parameters */
-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
 	protlen = ntohs(eth_hdr->et_protlen);
 	if (protlen == 0x8100) {
 		hdr_size += 4;
-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size - 2]);
 	}
 	if (header_size)
 		*header_size = hdr_size;
@@ -621,17 +624,22 @@ 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 < receive_lengths[rx_packet_idx]) {
 		/* Packet doesn't fit, try again with bigger buffer */
-		*buffer_size = net_rx_packet_len;
+		*buffer_size = receive_lengths[rx_packet_idx];
 		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, receive_buffer[rx_packet_idx],
+	       receive_lengths[rx_packet_idx]);
+	*buffer_size = receive_lengths[rx_packet_idx];
+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
+	rx_packet_num--;
+	if (rx_packet_num)
+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+	else
+		wait_for_packet->is_signaled = true;
 out:
 	return EFI_EXIT(ret);
 }
@@ -664,7 +672,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
  */
 static void efi_net_push(void *pkt, int len)
 {
-	new_rx_packet = true;
+	int rx_packet_next;
+
+	/* Check that we at least received an Ethernet header */
+	if (len < sizeof(struct ethernet_hdr))
+		return;
+
+	/* Check that the buffer won't overflow */
+	if (len > PKTSIZE_ALIGN)
+		return;
+
+	/* Can't store more than pre-alloced buffer */
+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
+		return;
+
+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
+	    ETH_PACKETS_BATCH_RECV;
+	memcpy(receive_buffer[rx_packet_next], pkt, len);
+	receive_lengths[rx_packet_next] = len;
+
+	rx_packet_num++;
 }
 
 /**
@@ -689,20 +716,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 (!rx_packet_num) {
 		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 (rx_packet_num) {
+			this->int_status |=
+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+			wait_for_packet->is_signaled = true;
 		}
 	}
 out:
@@ -830,6 +851,7 @@ efi_status_t efi_net_register(void)
 {
 	struct efi_net_obj *netobj = NULL;
 	efi_status_t r;
+	int i;
 
 	if (!eth_get_dev()) {
 		/* No network device active, don't expose any */
@@ -847,6 +869,21 @@ efi_status_t efi_net_register(void)
 		goto out_of_resources;
 	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
 
+	/* Allocate a number of receive buffers */
+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
+				sizeof(*receive_buffer));
+	if (!receive_buffer)
+		goto out_of_resources;
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
+		if (!receive_buffer[i])
+			goto out_of_resources;
+	}
+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
+				 sizeof(*receive_lengths));
+	if (!receive_lengths)
+		goto out_of_resources;
+
 	/* Hook net up to the device list */
 	efi_add_handle(&netobj->header);
 
@@ -941,7 +978,12 @@ failure_to_add_protocol:
 	return r;
 out_of_resources:
 	free(netobj);
-	/* free(transmit_buffer) not needed yet */
+	free(transmit_buffer);
+	if (receive_buffer)
+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
+			free(receive_buffer[i]);
+	free(receive_buffer);
+	free(receive_lengths);
 	printf("ERROR: Out of memory\n");
 	return EFI_OUT_OF_RESOURCES;
 }
-- 
2.28.0



More information about the U-Boot mailing list