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

[Patrick Wildt]

On Wed, Oct 07, 2020 at 12:45:17AM +0200, Heinrich Schuchardt wrote:
> Am 7. Oktober 2020 00:30:51 MESZ schrieb Patrick Wildt <patrick at blueri.se>:
> >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.  Make that magic
> >number a define, so it only needs to be changed in one place.
> 
> I am missing the maintainer for the network patch on cc.
> 
> Moving the constant should be a separate patch.
> 

Sure, will split it up and send it to that one as well.

> 
> >
> >Signed-off-by: Patrick Wildt <patrick at blueri.se>
> >---
> > include/net.h            |  3 ++
> > lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
> > net/eth-uclass.c         |  2 +-
> > 3 files changed, 69 insertions(+), 24 deletions(-)
> >
> >diff --git a/include/net.h b/include/net.h
> >index 219107194f..eab4ebdd38 100644
> >--- a/include/net.h
> >+++ b/include/net.h
> >@@ -44,6 +44,9 @@ struct udevice;
> > 
> > #define PKTALIGN	ARCH_DMA_MINALIGN
> > 
> >+/* Number of packets processed together */
> >+#define ETH_PACKETS_BATCH_RECV	32
> >+
> > /* ARP hardware address length */
> > #define ARP_HLEN 6
> > /*
> >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;
> 
> I am not sure if this can happen. Is it preferable to drop the newest message instead of the oldest one (using a circular buffer)? Some payloads run TCP/IP on our stack.

Network cards buffer also drop the newest ones.  And that's the right
way to do it, because otherwise it is possible to receive packets out
of order, which isn't really nice with TCP/IP.  Yes, even in regular
networks it can happen that one packet in the middle can be lost, but
if it's a number of packets we'd rather drop the newest.

Best regards,
Patrick

> Best regards
> 
> Heinrich
> 
> >+
> >+	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;
> > }
> >diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> >index 396418eb39..963a0beaab 100644
> >--- a/net/eth-uclass.c
> >+++ b/net/eth-uclass.c
> >@@ -380,7 +380,7 @@ int eth_rx(void)
> > 
> > 	/* Process up to 32 packets at one time */
> > 	flags = ETH_RECV_CHECK_DEVICE;
> >-	for (i = 0; i < 32; i++) {
> >+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
> > 		ret = eth_get_ops(current)->recv(current, flags, &packet);
> > 		flags = 0;
> > 		if (ret > 0)
>