From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick Wildt Date: Tue, 6 Oct 2020 18:32:24 +0200 Subject: efi_loader: fix free() before use in RX path In-Reply-To: <20201006145631.GA2844@jump> References: <20201006145631.GA2844@jump> Message-ID: <20201006163224.GA2857@jump> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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: