From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick Wildt Date: Wed, 7 Oct 2020 00:29:34 +0200 Subject: efi_loader: fix free() before use in RX path In-Reply-To: References: <20201006145631.GA2844@jump> <20201006163224.GA2857@jump> Message-ID: <20201006222934.GA15578@nox.fritz.box> 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 10:26:44PM +0200, Heinrich Schuchardt wrote: > On 10/6/20 6:32 PM, Patrick Wildt wrote: > > 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()? > > > > Thanks for your analysis. > > Could you, please, send a patch with a proper commit message. > > Yes, efi_net_stop() and efi_net_reset() both should empty the queue. > > > 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; > > When leaving efi_net_receive() you have to ensure that > packet->is_signaled reflects the fill level of the queue. > > I guess the following will suffice. > > else > packet->is_signaled = true; > > > 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); > > Network is time critical. We should try to eliminate malloc(), > list_add_tail(), free(). Performance would be better if we move the > magic number 32 to include/net.h and create a buffer with the required > number of slots beforehand. > > Best regards > > Heinrich Thank you for the quick reply! I will send a proper patch as reply to this in a minute and I hope patchwork or so likes it... Please let me know if that is more what you imagine. Patrick > > } > > > > /** > > @@ -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: > > >