efi_loader: fix free() before use in RX path

[Heinrich Schuchardt]

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

>  }
>
>  /**
> @@ -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:
>