From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick Wildt Date: Tue, 6 Oct 2020 16:56:31 +0200 Subject: efi_loader: fix free() before use in RX path Message-ID: <20201006145631.GA2844@jump> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 Signed-off-by: Patrick Wildt diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 22f0123eca..6e3c29cba2 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -25,6 +25,8 @@ 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 uchar *efi_net_rx_packet; +static int efi_net_rx_packet_len; static void *new_tx_packet; static void *transmit_buffer; @@ -607,11 +609,11 @@ static efi_status_t EFIAPI efi_net_receive goto out; } /* Fill export parameters */ - eth_hdr = (struct ethernet_hdr *)net_rx_packet; + eth_hdr = (struct ethernet_hdr *)efi_net_rx_packet; protlen = ntohs(eth_hdr->et_protlen); if (protlen == 0x8100) { hdr_size += 4; - protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]); + protlen = ntohs(*(u16 *)&efi_net_rx_packet[hdr_size - 2]); } if (header_size) *header_size = hdr_size; @@ -621,16 +623,19 @@ 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 < efi_net_rx_packet_len) { /* Packet doesn't fit, try again with bigger buffer */ - *buffer_size = net_rx_packet_len; + *buffer_size = efi_net_rx_packet_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; + memcpy(buffer, efi_net_rx_packet, efi_net_rx_packet_len); + *buffer_size = efi_net_rx_packet_len; new_rx_packet = 0; + free(efi_net_rx_packet); + efi_net_rx_packet = NULL; + efi_net_rx_packet_len = 0; this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT; out: return EFI_EXIT(ret); @@ -664,6 +669,13 @@ void efi_net_set_dhcp_ack(void *pkt, int len) */ static void efi_net_push(void *pkt, int len) { + efi_net_rx_packet = malloc(len); + if (efi_net_rx_packet == NULL) + return; + + memcpy(efi_net_rx_packet, pkt, len); + efi_net_rx_packet_len = len; + new_rx_packet = true; }