* efi_loader: fix free() before use in RX path
@ 2020-10-06 14:56 Patrick Wildt
2020-10-06 16:32 ` Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 14:56 UTC (permalink / raw
To: u-boot
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;
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* efi_loader: fix free() before use in RX path
2020-10-06 14:56 efi_loader: fix free() before use in RX path Patrick Wildt
@ 2020-10-06 16:32 ` Patrick Wildt
2020-10-06 20:26 ` Heinrich Schuchardt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 16:32 UTC (permalink / raw
To: u-boot
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:
^ permalink raw reply related [flat|nested] 16+ messages in thread
* efi_loader: fix free() before use in RX path
2020-10-06 16:32 ` Patrick Wildt
@ 2020-10-06 20:26 ` Heinrich Schuchardt
2020-10-06 22:29 ` Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2020-10-06 20:26 UTC (permalink / raw
To: u-boot
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:
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* efi_loader: fix free() before use in RX path
2020-10-06 20:26 ` Heinrich Schuchardt
@ 2020-10-06 22:29 ` Patrick Wildt
2020-10-06 22:30 ` [PATCH] efi_loader: fix use after free in receive path Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 22:29 UTC (permalink / raw
To: u-boot
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:
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] efi_loader: fix use after free in receive path
2020-10-06 22:29 ` Patrick Wildt
@ 2020-10-06 22:30 ` Patrick Wildt
2020-10-06 22:45 ` Heinrich Schuchardt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 22:30 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] efi_loader: fix use after free in receive path
2020-10-06 22:30 ` [PATCH] efi_loader: fix use after free in receive path Patrick Wildt
@ 2020-10-06 22:45 ` Heinrich Schuchardt
2020-10-06 22:51 ` Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2020-10-06 22:45 UTC (permalink / raw
To: u-boot
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)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] efi_loader: fix use after free in receive path
2020-10-06 22:45 ` Heinrich Schuchardt
@ 2020-10-06 22:51 ` Patrick Wildt
2020-10-06 22:57 ` [PATCH v2 1/2] net: add a define for the number of packets received as batch Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 22:51 UTC (permalink / raw
To: u-boot
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)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] net: add a define for the number of packets received as batch
2020-10-06 22:51 ` Patrick Wildt
@ 2020-10-06 22:57 ` Patrick Wildt
2020-10-06 22:59 ` [PATCH v2 2/2] efi_loader: fix use after free in receive path Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 22:57 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] efi_loader: fix use after free in receive path
2020-10-06 22:57 ` [PATCH v2 1/2] net: add a define for the number of packets received as batch Patrick Wildt
@ 2020-10-06 22:59 ` Patrick Wildt
2020-10-07 9:03 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-06 22:59 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] net: add a define for the number of packets received as batch
2020-10-06 22:59 ` [PATCH v2 2/2] efi_loader: fix use after free in receive path Patrick Wildt
@ 2020-10-07 9:03 ` Patrick Wildt
2020-10-07 9:04 ` [PATCH v3 2/2] efi_loader: fix use after free in receive path Patrick Wildt
2020-10-07 10:51 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Heinrich Schuchardt
0 siblings, 2 replies; 16+ messages in thread
From: Patrick Wildt @ 2020-10-07 9:03 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] efi_loader: fix use after free in receive path
2020-10-07 9:03 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Patrick Wildt
@ 2020-10-07 9:04 ` Patrick Wildt
2020-10-07 13:26 ` Heinrich Schuchardt
2020-10-07 10:51 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Heinrich Schuchardt
1 sibling, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-10-07 9:04 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3 1/2] net: add a define for the number of packets received as batch
2020-10-07 9:03 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Patrick Wildt
2020-10-07 9:04 ` [PATCH v3 2/2] efi_loader: fix use after free in receive path Patrick Wildt
@ 2020-10-07 10:51 ` Heinrich Schuchardt
1 sibling, 0 replies; 16+ messages in thread
From: Heinrich Schuchardt @ 2020-10-07 10:51 UTC (permalink / raw
To: u-boot
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)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] efi_loader: fix use after free in receive path
2020-10-07 9:04 ` [PATCH v3 2/2] efi_loader: fix use after free in receive path Patrick Wildt
@ 2020-10-07 13:26 ` Heinrich Schuchardt
2020-11-20 22:56 ` Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2020-10-07 13:26 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] efi_loader: fix use after free in receive path
2020-10-07 13:26 ` Heinrich Schuchardt
@ 2020-11-20 22:56 ` Patrick Wildt
2020-11-20 23:31 ` Tom Rini
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Wildt @ 2020-11-20 22:56 UTC (permalink / raw
To: u-boot
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?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] efi_loader: fix use after free in receive path
2020-11-20 22:56 ` Patrick Wildt
@ 2020-11-20 23:31 ` Tom Rini
2020-11-21 1:36 ` Patrick Wildt
0 siblings, 1 reply; 16+ messages in thread
From: Tom Rini @ 2020-11-20 23:31 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/2] efi_loader: fix use after free in receive path
2020-11-20 23:31 ` Tom Rini
@ 2020-11-21 1:36 ` Patrick Wildt
0 siblings, 0 replies; 16+ messages in thread
From: Patrick Wildt @ 2020-11-21 1:36 UTC (permalink / raw
To: u-boot
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! :)
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-11-21 1:36 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-06 14:56 efi_loader: fix free() before use in RX path Patrick Wildt
2020-10-06 16:32 ` Patrick Wildt
2020-10-06 20:26 ` Heinrich Schuchardt
2020-10-06 22:29 ` Patrick Wildt
2020-10-06 22:30 ` [PATCH] efi_loader: fix use after free in receive path Patrick Wildt
2020-10-06 22:45 ` Heinrich Schuchardt
2020-10-06 22:51 ` Patrick Wildt
2020-10-06 22:57 ` [PATCH v2 1/2] net: add a define for the number of packets received as batch Patrick Wildt
2020-10-06 22:59 ` [PATCH v2 2/2] efi_loader: fix use after free in receive path Patrick Wildt
2020-10-07 9:03 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Patrick Wildt
2020-10-07 9:04 ` [PATCH v3 2/2] efi_loader: fix use after free in receive path Patrick Wildt
2020-10-07 13:26 ` Heinrich Schuchardt
2020-11-20 22:56 ` Patrick Wildt
2020-11-20 23:31 ` Tom Rini
2020-11-21 1:36 ` Patrick Wildt
2020-10-07 10:51 ` [PATCH v3 1/2] net: add a define for the number of packets received as batch Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.