efi_loader: fix free() before use in RX path

efi_loader: fix free() before use in RX path

[Patrick Wildt]

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

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;
 }
 

efi_loader: fix free() before use in RX path

[Patrick Wildt]

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 <patrick@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;
 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:

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@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:
>

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@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:
> >
> 

[PATCH] efi_loader: fix use after free in receive path

[Patrick Wildt]

With DM enabled the ethernet code will receive a packet, call
the push method that's set by the EFI network implementation
and then free the packet.  Unfortunately the push methods only
sets a flag that the packet needs to be handled, but the code
that provides the packet to an EFI application runs after the
packet has already been freed.

To rectify this issue, adjust the push method to accept the packet
and store it in a temporary buffer.  The EFI application then gets
the data copied from that buffer.  This way the packet is cached
until is is needed.

The DM Ethernet stack tries to receive 32 packets at once, thus
we better allocate as many buffers as the stack.  Make that magic
number a define, so it only needs to be changed in one place.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
 include/net.h            |  3 ++
 lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
 net/eth-uclass.c         |  2 +-
 3 files changed, 69 insertions(+), 24 deletions(-)

diff --git a/include/net.h b/include/net.h
index 219107194f..eab4ebdd38 100644
--- a/include/net.h
+++ b/include/net.h
@@ -44,6 +44,9 @@ struct udevice;
 
 #define PKTALIGN	ARCH_DMA_MINALIGN
 
+/* Number of packets processed together */
+#define ETH_PACKETS_BATCH_RECV	32
+
 /* ARP hardware address length */
 #define ARP_HLEN 6
 /*
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 22f0123eca..ed72df42b8 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -24,9 +24,12 @@ 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;
+static uchar **receive_buffer;
+static size_t *receive_lengths;
+static int rx_packet_idx;
+static int rx_packet_num;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -602,16 +605,16 @@ static efi_status_t EFIAPI efi_net_receive
 		break;
 	}
 
-	if (!new_rx_packet) {
+	if (!rx_packet_num) {
 		ret = EFI_NOT_READY;
 		goto out;
 	}
 	/* Fill export parameters */
-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
 	protlen = ntohs(eth_hdr->et_protlen);
 	if (protlen == 0x8100) {
 		hdr_size += 4;
-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size - 2]);
 	}
 	if (header_size)
 		*header_size = hdr_size;
@@ -621,17 +624,22 @@ 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 < receive_lengths[rx_packet_idx]) {
 		/* Packet doesn't fit, try again with bigger buffer */
-		*buffer_size = net_rx_packet_len;
+		*buffer_size = receive_lengths[rx_packet_idx];
 		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, receive_buffer[rx_packet_idx],
+	       receive_lengths[rx_packet_idx]);
+	*buffer_size = receive_lengths[rx_packet_idx];
+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
+	rx_packet_num--;
+	if (rx_packet_num)
+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+	else
+		wait_for_packet->is_signaled = true;
 out:
 	return EFI_EXIT(ret);
 }
@@ -664,7 +672,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
  */
 static void efi_net_push(void *pkt, int len)
 {
-	new_rx_packet = true;
+	int rx_packet_next;
+
+	/* Check that we at least received an Ethernet header */
+	if (len < sizeof(struct ethernet_hdr))
+		return;
+
+	/* Check that the buffer won't overflow */
+	if (len > PKTSIZE_ALIGN)
+		return;
+
+	/* Can't store more than pre-alloced buffer */
+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
+		return;
+
+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
+	    ETH_PACKETS_BATCH_RECV;
+	memcpy(receive_buffer[rx_packet_next], pkt, len);
+	receive_lengths[rx_packet_next] = len;
+
+	rx_packet_num++;
 }
 
 /**
@@ -689,20 +716,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 (!rx_packet_num) {
 		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 (rx_packet_num) {
+			this->int_status |=
+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+			wait_for_packet->is_signaled = true;
 		}
 	}
 out:
@@ -830,6 +851,7 @@ efi_status_t efi_net_register(void)
 {
 	struct efi_net_obj *netobj = NULL;
 	efi_status_t r;
+	int i;
 
 	if (!eth_get_dev()) {
 		/* No network device active, don't expose any */
@@ -847,6 +869,21 @@ efi_status_t efi_net_register(void)
 		goto out_of_resources;
 	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
 
+	/* Allocate a number of receive buffers */
+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
+				sizeof(*receive_buffer));
+	if (!receive_buffer)
+		goto out_of_resources;
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
+		if (!receive_buffer[i])
+			goto out_of_resources;
+	}
+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
+				 sizeof(*receive_lengths));
+	if (!receive_lengths)
+		goto out_of_resources;
+
 	/* Hook net up to the device list */
 	efi_add_handle(&netobj->header);
 
@@ -941,7 +978,12 @@ failure_to_add_protocol:
 	return r;
 out_of_resources:
 	free(netobj);
-	/* free(transmit_buffer) not needed yet */
+	free(transmit_buffer);
+	if (receive_buffer)
+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
+			free(receive_buffer[i]);
+	free(receive_buffer);
+	free(receive_lengths);
 	printf("ERROR: Out of memory\n");
 	return EFI_OUT_OF_RESOURCES;
 }
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 396418eb39..963a0beaab 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -380,7 +380,7 @@ int eth_rx(void)
 
 	/* Process up to 32 packets@one time */
 	flags = ETH_RECV_CHECK_DEVICE;
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
 		ret = eth_get_ops(current)->recv(current, flags, &packet);
 		flags = 0;
 		if (ret > 0)
-- 
2.28.0

[PATCH] efi_loader: fix use after free in receive path

[Heinrich Schuchardt]

Am 7. Oktober 2020 00:30:51 MESZ schrieb Patrick Wildt <patrick@blueri.se>:
>With DM enabled the ethernet code will receive a packet, call
>the push method that's set by the EFI network implementation
>and then free the packet.  Unfortunately the push methods only
>sets a flag that the packet needs to be handled, but the code
>that provides the packet to an EFI application runs after the
>packet has already been freed.
>
>To rectify this issue, adjust the push method to accept the packet
>and store it in a temporary buffer.  The EFI application then gets
>the data copied from that buffer.  This way the packet is cached
>until is is needed.
>
>The DM Ethernet stack tries to receive 32 packets at once, thus
>we better allocate as many buffers as the stack.  Make that magic
>number a define, so it only needs to be changed in one place.

I am missing the maintainer for the network patch on cc.

Moving the constant should be a separate patch.


>
>Signed-off-by: Patrick Wildt <patrick@blueri.se>
>---
> include/net.h            |  3 ++
> lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
> net/eth-uclass.c         |  2 +-
> 3 files changed, 69 insertions(+), 24 deletions(-)
>
>diff --git a/include/net.h b/include/net.h
>index 219107194f..eab4ebdd38 100644
>--- a/include/net.h
>+++ b/include/net.h
>@@ -44,6 +44,9 @@ struct udevice;
> 
> #define PKTALIGN	ARCH_DMA_MINALIGN
> 
>+/* Number of packets processed together */
>+#define ETH_PACKETS_BATCH_RECV	32
>+
> /* ARP hardware address length */
> #define ARP_HLEN 6
> /*
>diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
>index 22f0123eca..ed72df42b8 100644
>--- a/lib/efi_loader/efi_net.c
>+++ b/lib/efi_loader/efi_net.c
>@@ -24,9 +24,12 @@ 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;
>+static uchar **receive_buffer;
>+static size_t *receive_lengths;
>+static int rx_packet_idx;
>+static int rx_packet_num;
> 
> /*
>* The notification function of this event is called in every timer
>cycle
>@@ -602,16 +605,16 @@ static efi_status_t EFIAPI efi_net_receive
> 		break;
> 	}
> 
>-	if (!new_rx_packet) {
>+	if (!rx_packet_num) {
> 		ret = EFI_NOT_READY;
> 		goto out;
> 	}
> 	/* Fill export parameters */
>-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
>+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
> 	protlen = ntohs(eth_hdr->et_protlen);
> 	if (protlen == 0x8100) {
> 		hdr_size += 4;
>-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
>+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size -
>2]);
> 	}
> 	if (header_size)
> 		*header_size = hdr_size;
>@@ -621,17 +624,22 @@ 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 < receive_lengths[rx_packet_idx]) {
> 		/* Packet doesn't fit, try again with bigger buffer */
>-		*buffer_size = net_rx_packet_len;
>+		*buffer_size = receive_lengths[rx_packet_idx];
> 		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, receive_buffer[rx_packet_idx],
>+	       receive_lengths[rx_packet_idx]);
>+	*buffer_size = receive_lengths[rx_packet_idx];
>+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
>+	rx_packet_num--;
>+	if (rx_packet_num)
>+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>+	else
>+		wait_for_packet->is_signaled = true;
> out:
> 	return EFI_EXIT(ret);
> }
>@@ -664,7 +672,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
>  */
> static void efi_net_push(void *pkt, int len)
> {
>-	new_rx_packet = true;
>+	int rx_packet_next;
>+
>+	/* Check that we at least received an Ethernet header */
>+	if (len < sizeof(struct ethernet_hdr))
>+		return;
>+
>+	/* Check that the buffer won't overflow */
>+	if (len > PKTSIZE_ALIGN)
>+		return;
>+
>+	/* Can't store more than pre-alloced buffer */
>+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
>+		return;

I am not sure if this can happen. Is it preferable to drop the newest message instead of the oldest one (using a circular buffer)? Some payloads run TCP/IP on our stack.

Best regards

Heinrich

>+
>+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
>+	    ETH_PACKETS_BATCH_RECV;
>+	memcpy(receive_buffer[rx_packet_next], pkt, len);
>+	receive_lengths[rx_packet_next] = len;
>+
>+	rx_packet_num++;
> }
> 
> /**
>@@ -689,20 +716,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 (!rx_packet_num) {
> 		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 (rx_packet_num) {
>+			this->int_status |=
>+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
>+			wait_for_packet->is_signaled = true;
> 		}
> 	}
> out:
>@@ -830,6 +851,7 @@ efi_status_t efi_net_register(void)
> {
> 	struct efi_net_obj *netobj = NULL;
> 	efi_status_t r;
>+	int i;
> 
> 	if (!eth_get_dev()) {
> 		/* No network device active, don't expose any */
>@@ -847,6 +869,21 @@ efi_status_t efi_net_register(void)
> 		goto out_of_resources;
>	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
> 
>+	/* Allocate a number of receive buffers */
>+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
>+				sizeof(*receive_buffer));
>+	if (!receive_buffer)
>+		goto out_of_resources;
>+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
>+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
>+		if (!receive_buffer[i])
>+			goto out_of_resources;
>+	}
>+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
>+				 sizeof(*receive_lengths));
>+	if (!receive_lengths)
>+		goto out_of_resources;
>+
> 	/* Hook net up to the device list */
> 	efi_add_handle(&netobj->header);
> 
>@@ -941,7 +978,12 @@ failure_to_add_protocol:
> 	return r;
> out_of_resources:
> 	free(netobj);
>-	/* free(transmit_buffer) not needed yet */
>+	free(transmit_buffer);
>+	if (receive_buffer)
>+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
>+			free(receive_buffer[i]);
>+	free(receive_buffer);
>+	free(receive_lengths);
> 	printf("ERROR: Out of memory\n");
> 	return EFI_OUT_OF_RESOURCES;
> }
>diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>index 396418eb39..963a0beaab 100644
>--- a/net/eth-uclass.c
>+++ b/net/eth-uclass.c
>@@ -380,7 +380,7 @@ int eth_rx(void)
> 
> 	/* Process up to 32 packets at one time */
> 	flags = ETH_RECV_CHECK_DEVICE;
>-	for (i = 0; i < 32; i++) {
>+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
> 		ret = eth_get_ops(current)->recv(current, flags, &packet);
> 		flags = 0;
> 		if (ret > 0)

[PATCH] efi_loader: fix use after free in receive path

[Patrick Wildt]

On Wed, Oct 07, 2020 at 12:45:17AM +0200, Heinrich Schuchardt wrote:
> Am 7. Oktober 2020 00:30:51 MESZ schrieb Patrick Wildt <patrick@blueri.se>:
> >With DM enabled the ethernet code will receive a packet, call
> >the push method that's set by the EFI network implementation
> >and then free the packet.  Unfortunately the push methods only
> >sets a flag that the packet needs to be handled, but the code
> >that provides the packet to an EFI application runs after the
> >packet has already been freed.
> >
> >To rectify this issue, adjust the push method to accept the packet
> >and store it in a temporary buffer.  The EFI application then gets
> >the data copied from that buffer.  This way the packet is cached
> >until is is needed.
> >
> >The DM Ethernet stack tries to receive 32 packets at once, thus
> >we better allocate as many buffers as the stack.  Make that magic
> >number a define, so it only needs to be changed in one place.
> 
> I am missing the maintainer for the network patch on cc.
> 
> Moving the constant should be a separate patch.
> 

Sure, will split it up and send it to that one as well.

> 
> >
> >Signed-off-by: Patrick Wildt <patrick@blueri.se>
> >---
> > include/net.h            |  3 ++
> > lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
> > net/eth-uclass.c         |  2 +-
> > 3 files changed, 69 insertions(+), 24 deletions(-)
> >
> >diff --git a/include/net.h b/include/net.h
> >index 219107194f..eab4ebdd38 100644
> >--- a/include/net.h
> >+++ b/include/net.h
> >@@ -44,6 +44,9 @@ struct udevice;
> > 
> > #define PKTALIGN	ARCH_DMA_MINALIGN
> > 
> >+/* Number of packets processed together */
> >+#define ETH_PACKETS_BATCH_RECV	32
> >+
> > /* ARP hardware address length */
> > #define ARP_HLEN 6
> > /*
> >diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> >index 22f0123eca..ed72df42b8 100644
> >--- a/lib/efi_loader/efi_net.c
> >+++ b/lib/efi_loader/efi_net.c
> >@@ -24,9 +24,12 @@ 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;
> >+static uchar **receive_buffer;
> >+static size_t *receive_lengths;
> >+static int rx_packet_idx;
> >+static int rx_packet_num;
> > 
> > /*
> >* The notification function of this event is called in every timer
> >cycle
> >@@ -602,16 +605,16 @@ static efi_status_t EFIAPI efi_net_receive
> > 		break;
> > 	}
> > 
> >-	if (!new_rx_packet) {
> >+	if (!rx_packet_num) {
> > 		ret = EFI_NOT_READY;
> > 		goto out;
> > 	}
> > 	/* Fill export parameters */
> >-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
> >+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
> > 	protlen = ntohs(eth_hdr->et_protlen);
> > 	if (protlen == 0x8100) {
> > 		hdr_size += 4;
> >-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
> >+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size -
> >2]);
> > 	}
> > 	if (header_size)
> > 		*header_size = hdr_size;
> >@@ -621,17 +624,22 @@ 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 < receive_lengths[rx_packet_idx]) {
> > 		/* Packet doesn't fit, try again with bigger buffer */
> >-		*buffer_size = net_rx_packet_len;
> >+		*buffer_size = receive_lengths[rx_packet_idx];
> > 		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, receive_buffer[rx_packet_idx],
> >+	       receive_lengths[rx_packet_idx]);
> >+	*buffer_size = receive_lengths[rx_packet_idx];
> >+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
> >+	rx_packet_num--;
> >+	if (rx_packet_num)
> >+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> >+	else
> >+		wait_for_packet->is_signaled = true;
> > out:
> > 	return EFI_EXIT(ret);
> > }
> >@@ -664,7 +672,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
> >  */
> > static void efi_net_push(void *pkt, int len)
> > {
> >-	new_rx_packet = true;
> >+	int rx_packet_next;
> >+
> >+	/* Check that we at least received an Ethernet header */
> >+	if (len < sizeof(struct ethernet_hdr))
> >+		return;
> >+
> >+	/* Check that the buffer won't overflow */
> >+	if (len > PKTSIZE_ALIGN)
> >+		return;
> >+
> >+	/* Can't store more than pre-alloced buffer */
> >+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
> >+		return;
> 
> I am not sure if this can happen. Is it preferable to drop the newest message instead of the oldest one (using a circular buffer)? Some payloads run TCP/IP on our stack.

Network cards buffer also drop the newest ones.  And that's the right
way to do it, because otherwise it is possible to receive packets out
of order, which isn't really nice with TCP/IP.  Yes, even in regular
networks it can happen that one packet in the middle can be lost, but
if it's a number of packets we'd rather drop the newest.

Best regards,
Patrick

> Best regards
> 
> Heinrich
> 
> >+
> >+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
> >+	    ETH_PACKETS_BATCH_RECV;
> >+	memcpy(receive_buffer[rx_packet_next], pkt, len);
> >+	receive_lengths[rx_packet_next] = len;
> >+
> >+	rx_packet_num++;
> > }
> > 
> > /**
> >@@ -689,20 +716,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 (!rx_packet_num) {
> > 		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 (rx_packet_num) {
> >+			this->int_status |=
> >+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> >+			wait_for_packet->is_signaled = true;
> > 		}
> > 	}
> > out:
> >@@ -830,6 +851,7 @@ efi_status_t efi_net_register(void)
> > {
> > 	struct efi_net_obj *netobj = NULL;
> > 	efi_status_t r;
> >+	int i;
> > 
> > 	if (!eth_get_dev()) {
> > 		/* No network device active, don't expose any */
> >@@ -847,6 +869,21 @@ efi_status_t efi_net_register(void)
> > 		goto out_of_resources;
> >	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
> > 
> >+	/* Allocate a number of receive buffers */
> >+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
> >+				sizeof(*receive_buffer));
> >+	if (!receive_buffer)
> >+		goto out_of_resources;
> >+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
> >+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
> >+		if (!receive_buffer[i])
> >+			goto out_of_resources;
> >+	}
> >+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
> >+				 sizeof(*receive_lengths));
> >+	if (!receive_lengths)
> >+		goto out_of_resources;
> >+
> > 	/* Hook net up to the device list */
> > 	efi_add_handle(&netobj->header);
> > 
> >@@ -941,7 +978,12 @@ failure_to_add_protocol:
> > 	return r;
> > out_of_resources:
> > 	free(netobj);
> >-	/* free(transmit_buffer) not needed yet */
> >+	free(transmit_buffer);
> >+	if (receive_buffer)
> >+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
> >+			free(receive_buffer[i]);
> >+	free(receive_buffer);
> >+	free(receive_lengths);
> > 	printf("ERROR: Out of memory\n");
> > 	return EFI_OUT_OF_RESOURCES;
> > }
> >diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> >index 396418eb39..963a0beaab 100644
> >--- a/net/eth-uclass.c
> >+++ b/net/eth-uclass.c
> >@@ -380,7 +380,7 @@ int eth_rx(void)
> > 
> > 	/* Process up to 32 packets at one time */
> > 	flags = ETH_RECV_CHECK_DEVICE;
> >-	for (i = 0; i < 32; i++) {
> >+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
> > 		ret = eth_get_ops(current)->recv(current, flags, &packet);
> > 		flags = 0;
> > 		if (ret > 0)
> 

[PATCH v2 1/2] net: add a define for the number of packets received as batch

[Patrick Wildt]

With a define for the magic number of packets received as batch
we can make sure that the EFI network stack caches the same amount
of packets.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
 include/net.h    | 3 +++
 net/eth-uclass.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index 219107194f..eab4ebdd38 100644
--- a/include/net.h
+++ b/include/net.h
@@ -44,6 +44,9 @@ struct udevice;
 
 #define PKTALIGN	ARCH_DMA_MINALIGN
 
+/* Number of packets processed together */
+#define ETH_PACKETS_BATCH_RECV	32
+
 /* ARP hardware address length */
 #define ARP_HLEN 6
 /*
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 396418eb39..963a0beaab 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -380,7 +380,7 @@ int eth_rx(void)
 
 	/* Process up to 32 packets at one time */
 	flags = ETH_RECV_CHECK_DEVICE;
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
 		ret = eth_get_ops(current)->recv(current, flags, &packet);
 		flags = 0;
 		if (ret > 0)
-- 
2.28.0

[PATCH v2 2/2] efi_loader: fix use after free in receive path

[Patrick Wildt]

With DM enabled the ethernet code will receive a packet, call
the push method that's set by the EFI network implementation
and then free the packet.  Unfortunately the push methods only
sets a flag that the packet needs to be handled, but the code
that provides the packet to an EFI application runs after the
packet has already been freed.

To rectify this issue, adjust the push method to accept the packet
and store it in a temporary buffer.  The EFI application then gets
the data copied from that buffer.  This way the packet is cached
until is is needed.

The DM Ethernet stack tries to receive 32 packets at once, thus
we better allocate as many buffers as the stack.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---
 lib/efi_loader/efi_net.c | 88 +++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 22f0123eca..ed72df42b8 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -24,9 +24,12 @@ 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;
+static uchar **receive_buffer;
+static size_t *receive_lengths;
+static int rx_packet_idx;
+static int rx_packet_num;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -602,16 +605,16 @@ static efi_status_t EFIAPI efi_net_receive
 		break;
 	}
 
-	if (!new_rx_packet) {
+	if (!rx_packet_num) {
 		ret = EFI_NOT_READY;
 		goto out;
 	}
 	/* Fill export parameters */
-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
 	protlen = ntohs(eth_hdr->et_protlen);
 	if (protlen == 0x8100) {
 		hdr_size += 4;
-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size - 2]);
 	}
 	if (header_size)
 		*header_size = hdr_size;
@@ -621,17 +624,22 @@ 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 < receive_lengths[rx_packet_idx]) {
 		/* Packet doesn't fit, try again with bigger buffer */
-		*buffer_size = net_rx_packet_len;
+		*buffer_size = receive_lengths[rx_packet_idx];
 		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, receive_buffer[rx_packet_idx],
+	       receive_lengths[rx_packet_idx]);
+	*buffer_size = receive_lengths[rx_packet_idx];
+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
+	rx_packet_num--;
+	if (rx_packet_num)
+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+	else
+		wait_for_packet->is_signaled = true;
 out:
 	return EFI_EXIT(ret);
 }
@@ -664,7 +672,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
  */
 static void efi_net_push(void *pkt, int len)
 {
-	new_rx_packet = true;
+	int rx_packet_next;
+
+	/* Check that we at least received an Ethernet header */
+	if (len < sizeof(struct ethernet_hdr))
+		return;
+
+	/* Check that the buffer won't overflow */
+	if (len > PKTSIZE_ALIGN)
+		return;
+
+	/* Can't store more than pre-alloced buffer */
+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
+		return;
+
+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
+	    ETH_PACKETS_BATCH_RECV;
+	memcpy(receive_buffer[rx_packet_next], pkt, len);
+	receive_lengths[rx_packet_next] = len;
+
+	rx_packet_num++;
 }
 
 /**
@@ -689,20 +716,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 (!rx_packet_num) {
 		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 (rx_packet_num) {
+			this->int_status |=
+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+			wait_for_packet->is_signaled = true;
 		}
 	}
 out:
@@ -830,6 +851,7 @@ efi_status_t efi_net_register(void)
 {
 	struct efi_net_obj *netobj = NULL;
 	efi_status_t r;
+	int i;
 
 	if (!eth_get_dev()) {
 		/* No network device active, don't expose any */
@@ -847,6 +869,21 @@ efi_status_t efi_net_register(void)
 		goto out_of_resources;
 	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
 
+	/* Allocate a number of receive buffers */
+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
+				sizeof(*receive_buffer));
+	if (!receive_buffer)
+		goto out_of_resources;
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
+		if (!receive_buffer[i])
+			goto out_of_resources;
+	}
+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
+				 sizeof(*receive_lengths));
+	if (!receive_lengths)
+		goto out_of_resources;
+
 	/* Hook net up to the device list */
 	efi_add_handle(&netobj->header);
 
@@ -941,7 +978,12 @@ failure_to_add_protocol:
 	return r;
 out_of_resources:
 	free(netobj);
-	/* free(transmit_buffer) not needed yet */
+	free(transmit_buffer);
+	if (receive_buffer)
+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
+			free(receive_buffer[i]);
+	free(receive_buffer);
+	free(receive_lengths);
 	printf("ERROR: Out of memory\n");
 	return EFI_OUT_OF_RESOURCES;
 }
-- 
2.28.0

[PATCH v3 1/2] net: add a define for the number of packets received as batch

[Patrick Wildt]

With a define for the magic number of packets received as batch
we can make sure that the EFI network stack caches the same amount
of packets.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---

Changes in v3:
- Simple rebase to resend patches together.

Changes in v2:
- Split this commit out of another one.

 include/net.h    | 3 +++
 net/eth-uclass.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index 219107194f..eab4ebdd38 100644
--- a/include/net.h
+++ b/include/net.h
@@ -44,6 +44,9 @@ struct udevice;
 
 #define PKTALIGN	ARCH_DMA_MINALIGN
 
+/* Number of packets processed together */
+#define ETH_PACKETS_BATCH_RECV	32
+
 /* ARP hardware address length */
 #define ARP_HLEN 6
 /*
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 396418eb39..963a0beaab 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -380,7 +380,7 @@ int eth_rx(void)
 
 	/* Process up to 32 packets at one time */
 	flags = ETH_RECV_CHECK_DEVICE;
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
 		ret = eth_get_ops(current)->recv(current, flags, &packet);
 		flags = 0;
 		if (ret > 0)
-- 
2.28.0

[PATCH v3 2/2] efi_loader: fix use after free in receive path

[Patrick Wildt]

With DM enabled the ethernet code will receive a packet, call
the push method that's set by the EFI network implementation
and then free the packet.  Unfortunately the push methods only
sets a flag that the packet needs to be handled, but the code
that provides the packet to an EFI application runs after the
packet has already been freed.

To rectify this issue, adjust the push method to accept the packet
and store it in a temporary buffer.  The EFI application then gets
the data copied from that buffer.  This way the packet is cached
until is is needed.

The DM Ethernet stack tries to receive 32 packets at once, thus
we better allocate as many buffers as the stack.

Signed-off-by: Patrick Wildt <patrick@blueri.se>
---

Changes from v2:
- Inverted the logic of efi_net_receive() reflecting the fill level,
  since in v2 it was the wrong way around.
- Clearing the packet queue by simply setting the amount of packets
  in the queue to zero.

Changes from v1:
- Reimplemented the buffer handling approach to use pre-allocated
  buffers.

 lib/efi_loader/efi_net.c | 92 ++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
index 22f0123eca..69276b275d 100644
--- a/lib/efi_loader/efi_net.c
+++ b/lib/efi_loader/efi_net.c
@@ -24,9 +24,12 @@ 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;
+static uchar **receive_buffer;
+static size_t *receive_lengths;
+static int rx_packet_idx;
+static int rx_packet_num;
 
 /*
  * The notification function of this event is called in every timer cycle
@@ -115,6 +118,8 @@ static efi_status_t EFIAPI efi_net_stop(struct efi_simple_network *this)
 	} else {
 		/* Disable hardware and put it into the reset state */
 		eth_halt();
+		/* Clear cache of packets */
+		rx_packet_num = 0;
 		this->mode->state = EFI_NETWORK_STOPPED;
 	}
 out:
@@ -160,6 +165,8 @@ static efi_status_t EFIAPI efi_net_initialize(struct efi_simple_network *this,
 	net_init();
 	/* Disable hardware and put it into the reset state */
 	eth_halt();
+	/* Clear cache of packets */
+	rx_packet_num = 0;
 	/* Set current device according to environment variables */
 	eth_set_current();
 	/* Get hardware ready for send and receive operations */
@@ -602,16 +609,16 @@ static efi_status_t EFIAPI efi_net_receive
 		break;
 	}
 
-	if (!new_rx_packet) {
+	if (!rx_packet_num) {
 		ret = EFI_NOT_READY;
 		goto out;
 	}
 	/* Fill export parameters */
-	eth_hdr = (struct ethernet_hdr *)net_rx_packet;
+	eth_hdr = (struct ethernet_hdr *)receive_buffer[rx_packet_idx];
 	protlen = ntohs(eth_hdr->et_protlen);
 	if (protlen == 0x8100) {
 		hdr_size += 4;
-		protlen = ntohs(*(u16 *)&net_rx_packet[hdr_size - 2]);
+		protlen = ntohs(*(u16 *)&receive_buffer[rx_packet_idx][hdr_size - 2]);
 	}
 	if (header_size)
 		*header_size = hdr_size;
@@ -621,17 +628,22 @@ 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 < receive_lengths[rx_packet_idx]) {
 		/* Packet doesn't fit, try again with bigger buffer */
-		*buffer_size = net_rx_packet_len;
+		*buffer_size = receive_lengths[rx_packet_idx];
 		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, receive_buffer[rx_packet_idx],
+	       receive_lengths[rx_packet_idx]);
+	*buffer_size = receive_lengths[rx_packet_idx];
+	rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
+	rx_packet_num--;
+	if (rx_packet_num)
+		wait_for_packet->is_signaled = true;
+	else
+		this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
 out:
 	return EFI_EXIT(ret);
 }
@@ -664,7 +676,26 @@ void efi_net_set_dhcp_ack(void *pkt, int len)
  */
 static void efi_net_push(void *pkt, int len)
 {
-	new_rx_packet = true;
+	int rx_packet_next;
+
+	/* Check that we at least received an Ethernet header */
+	if (len < sizeof(struct ethernet_hdr))
+		return;
+
+	/* Check that the buffer won't overflow */
+	if (len > PKTSIZE_ALIGN)
+		return;
+
+	/* Can't store more than pre-alloced buffer */
+	if (rx_packet_num >= ETH_PACKETS_BATCH_RECV)
+		return;
+
+	rx_packet_next = (rx_packet_idx + rx_packet_num) %
+	    ETH_PACKETS_BATCH_RECV;
+	memcpy(receive_buffer[rx_packet_next], pkt, len);
+	receive_lengths[rx_packet_next] = len;
+
+	rx_packet_num++;
 }
 
 /**
@@ -689,20 +720,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 (!rx_packet_num) {
 		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 (rx_packet_num) {
+			this->int_status |=
+				EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
+			wait_for_packet->is_signaled = true;
 		}
 	}
 out:
@@ -830,6 +855,7 @@ efi_status_t efi_net_register(void)
 {
 	struct efi_net_obj *netobj = NULL;
 	efi_status_t r;
+	int i;
 
 	if (!eth_get_dev()) {
 		/* No network device active, don't expose any */
@@ -847,6 +873,21 @@ efi_status_t efi_net_register(void)
 		goto out_of_resources;
 	transmit_buffer = (void *)ALIGN((uintptr_t)transmit_buffer, PKTALIGN);
 
+	/* Allocate a number of receive buffers */
+	receive_buffer = calloc(ETH_PACKETS_BATCH_RECV,
+				sizeof(*receive_buffer));
+	if (!receive_buffer)
+		goto out_of_resources;
+	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
+		receive_buffer[i] = malloc(PKTSIZE_ALIGN);
+		if (!receive_buffer[i])
+			goto out_of_resources;
+	}
+	receive_lengths = calloc(ETH_PACKETS_BATCH_RECV,
+				 sizeof(*receive_lengths));
+	if (!receive_lengths)
+		goto out_of_resources;
+
 	/* Hook net up to the device list */
 	efi_add_handle(&netobj->header);
 
@@ -941,7 +982,12 @@ failure_to_add_protocol:
 	return r;
 out_of_resources:
 	free(netobj);
-	/* free(transmit_buffer) not needed yet */
+	free(transmit_buffer);
+	if (receive_buffer)
+		for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
+			free(receive_buffer[i]);
+	free(receive_buffer);
+	free(receive_lengths);
 	printf("ERROR: Out of memory\n");
 	return EFI_OUT_OF_RESOURCES;
 }
-- 
2.28.0

[PATCH v3 2/2] efi_loader: fix use after free in receive path

[Heinrich Schuchardt]

On 07.10.20 11:04, Patrick Wildt wrote:
> With DM enabled the ethernet code will receive a packet, call
> the push method that's set by the EFI network implementation
> and then free the packet.  Unfortunately the push methods only
> sets a flag that the packet needs to be handled, but the code
> that provides the packet to an EFI application runs after the
> packet has already been freed.
>
> To rectify this issue, adjust the push method to accept the packet
> and store it in a temporary buffer.  The EFI application then gets
> the data copied from that buffer.  This way the packet is cached
> until is is needed.
>
> The DM Ethernet stack tries to receive 32 packets at once, thus
> we better allocate as many buffers as the stack.
>
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH v3 2/2] efi_loader: fix use after free in receive path

[Patrick Wildt]

Am Wed, Oct 07, 2020 at 03:26:38PM +0200 schrieb Heinrich Schuchardt:
> On 07.10.20 11:04, Patrick Wildt wrote:
> > With DM enabled the ethernet code will receive a packet, call
> > the push method that's set by the EFI network implementation
> > and then free the packet.  Unfortunately the push methods only
> > sets a flag that the packet needs to be handled, but the code
> > that provides the packet to an EFI application runs after the
> > packet has already been freed.
> >
> > To rectify this issue, adjust the push method to accept the packet
> > and store it in a temporary buffer.  The EFI application then gets
> > the data copied from that buffer.  This way the packet is cached
> > until is is needed.
> >
> > The DM Ethernet stack tries to receive 32 packets at once, thus
> > we better allocate as many buffers as the stack.
> >
> > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Was this patchset ever merged?

[PATCH v3 2/2] efi_loader: fix use after free in receive path

[Tom Rini]

On Fri, Nov 20, 2020 at 11:56:27PM +0100, Patrick Wildt wrote:
> Am Wed, Oct 07, 2020 at 03:26:38PM +0200 schrieb Heinrich Schuchardt:
> > On 07.10.20 11:04, Patrick Wildt wrote:
> > > With DM enabled the ethernet code will receive a packet, call
> > > the push method that's set by the EFI network implementation
> > > and then free the packet.  Unfortunately the push methods only
> > > sets a flag that the packet needs to be handled, but the code
> > > that provides the packet to an EFI application runs after the
> > > packet has already been freed.
> > >
> > > To rectify this issue, adjust the push method to accept the packet
> > > and store it in a temporary buffer.  The EFI application then gets
> > > the data copied from that buffer.  This way the packet is cached
> > > until is is needed.
> > >
> > > The DM Ethernet stack tries to receive 32 packets at once, thus
> > > we better allocate as many buffers as the stack.
> > >
> > > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> > 
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> Was this patchset ever merged?

commit 42f804fbba2836e64f472306ff7616c812d5c54d
Author: Patrick Wildt <patrick@blueri.se>
Date:   Wed Oct 7 11:04:33 2020 +0200

    efi_loader: fix use after free in rece

is what I see atm.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201120/7a1c3817/attachment.sig>

[PATCH v3 2/2] efi_loader: fix use after free in receive path

[Patrick Wildt]

On Fri, Nov 20, 2020 at 06:31:52PM -0500, Tom Rini wrote:
> On Fri, Nov 20, 2020 at 11:56:27PM +0100, Patrick Wildt wrote:
> > Am Wed, Oct 07, 2020 at 03:26:38PM +0200 schrieb Heinrich Schuchardt:
> > > On 07.10.20 11:04, Patrick Wildt wrote:
> > > > With DM enabled the ethernet code will receive a packet, call
> > > > the push method that's set by the EFI network implementation
> > > > and then free the packet.  Unfortunately the push methods only
> > > > sets a flag that the packet needs to be handled, but the code
> > > > that provides the packet to an EFI application runs after the
> > > > packet has already been freed.
> > > >
> > > > To rectify this issue, adjust the push method to accept the packet
> > > > and store it in a temporary buffer.  The EFI application then gets
> > > > the data copied from that buffer.  This way the packet is cached
> > > > until is is needed.
> > > >
> > > > The DM Ethernet stack tries to receive 32 packets at once, thus
> > > > we better allocate as many buffers as the stack.
> > > >
> > > > Signed-off-by: Patrick Wildt <patrick@blueri.se>
> > > 
> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > 
> > Was this patchset ever merged?
> 
> commit 42f804fbba2836e64f472306ff7616c812d5c54d
> Author: Patrick Wildt <patrick@blueri.se>
> Date:   Wed Oct 7 11:04:33 2020 +0200
> 
>     efi_loader: fix use after free in rece
> 
> is what I see atm.

Ah, yes, sorry, thanks! :)

[PATCH v3 1/2] net: add a define for the number of packets received as batch

[Heinrich Schuchardt]

On 07.10.20 11:03, Patrick Wildt wrote:
> With a define for the magic number of packets received as batch
> we can make sure that the EFI network stack caches the same amount
> of packets.
>
> Signed-off-by: Patrick Wildt <patrick@blueri.se>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> Changes in v3:
> - Simple rebase to resend patches together.
>
> Changes in v2:
> - Split this commit out of another one.
>
>  include/net.h    | 3 +++
>  net/eth-uclass.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/net.h b/include/net.h
> index 219107194f..eab4ebdd38 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -44,6 +44,9 @@ struct udevice;
>
>  #define PKTALIGN	ARCH_DMA_MINALIGN
>
> +/* Number of packets processed together */
> +#define ETH_PACKETS_BATCH_RECV	32
> +
>  /* ARP hardware address length */
>  #define ARP_HLEN 6
>  /*
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 396418eb39..963a0beaab 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -380,7 +380,7 @@ int eth_rx(void)
>
>  	/* Process up to 32 packets at one time */
>  	flags = ETH_RECV_CHECK_DEVICE;
> -	for (i = 0; i < 32; i++) {
> +	for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) {
>  		ret = eth_get_ops(current)->recv(current, flags, &packet);
>  		flags = 0;
>  		if (ret > 0)
>