[PATCH] efi_loader: fix use after free in receive path
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Oct 7 00:45:17 CEST 2020
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.
>
>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.
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)
More information about the U-Boot
mailing list