efi_loader: fix free() before use in RX path

[Patrick Wildt]

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 <patrick at blueri.se>
> >
> > 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:
> >
>