[PATCH 0/1] tap: fix write-after-free and double free of intr_handle

[PATCH 0/1] tap: fix write-after-free and double free of intr_handle

[Quentin Armitage]

Starting and stopping a tap device results in the following output:
  tap_lsc_intr_handle_set(): intr callback unregister failed: -2
  free(): invalid pointer
and a core dump is generated due to abort() being called.

Although the stack backtrace below gives line numbers for dpdk-21.11, this problem
still occurs with the current HEAD of the development tree (commit 7615ec581).

The stack backtrace is:

#0  0x00007f80741d12a2 in raise () from /lib64/libc.so.6
#1  0x00007f80741ba8a4 in abort () from /lib64/libc.so.6
#2  0x00007f8074213ac7 in __libc_message () from /lib64/libc.so.6
#3  0x00007f807421b73c in malloc_printerr () from /lib64/libc.so.6
#4  0x00007f807421c97c in _int_free () from /lib64/libc.so.6
#5  0x00007f80742207a8 in free () from /lib64/libc.so.6
#6  0x00007f8074374bf5 in rte_intr_instance_free (intr_handle=intr_handle@entry=0x1003b2480) at ../lib/eal/common/eal_common_interrupts.c:184
#7  0x00007f8072bf16ce in tap_rx_intr_vec_uninstall (dev=dev@entry=0x7f80744ed480 <rte_eth_devices>) at ../drivers/net/tap/tap_intr.c:38
#8  0x00007f8072bf18d3 in tap_rx_intr_vec_set (dev=dev@entry=0x7f80744ed480 <rte_eth_devices>, set=set@entry=0) at ../drivers/net/tap/tap_intr.c:111
#9  0x00007f8072bea742 in tap_intr_handle_set (dev=dev@entry=0x7f80744ed480 <rte_eth_devices>, set=set@entry=0) at ../drivers/net/tap/rte_eth_tap.c:1727
#10 0x00007f8072bea7d0 in tap_dev_stop (dev=0x7f80744ed480 <rte_eth_devices>) at ../drivers/net/tap/rte_eth_tap.c:916
#11 0x00007f80744875a4 in rte_eth_dev_stop (port_id=<optimized out>) at ../lib/ethdev/rte_ethdev.c:1883
#12 0x000000000040158b in main (argc=4, argv=0x7ffd13cfc368) at tap_free.c:59

A sample program to demonstrate the problem is
=======================================================================
// Run as: build/tap_free --vdev=net_tap0,remote=PORT -l 0,1

#include <stdio.h>

#include <rte_eal.h>
#include <rte_ethdev.h>
#include <rte_mbuf.h>

int
main(int argc, char *argv[])
{
	uint16_t port_id;
	struct rte_mempool *mbuf_pool;
	struct rte_eth_conf port_conf;
	struct rte_eth_dev_info dev_info;
	uint16_t nb_rxd = 1024;
	uint16_t nb_txd = 1024;
	struct rte_eth_txconf txconf;

	if (rte_eal_init(argc, argv) < 0)
		rte_exit(EXIT_FAILURE, "Error with EAL initialization\n");

	if (rte_eth_dev_count_avail() < 1)
		rte_exit(EXIT_FAILURE, "Error: should have at least 1 port\n");

	port_id = rte_eth_find_next_owned_by(0, RTE_ETH_DEV_NO_OWNER);

	mbuf_pool = rte_pktmbuf_pool_create("mbuf_pool", 1023, 256, 0, RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id());

	if (!rte_eth_dev_is_valid_port(port_id))
		rte_exit(1, "a\n");

	memset(&port_conf, 0, sizeof(struct rte_eth_conf));

	if (rte_eth_dev_info_get(port_id, &dev_info))
		rte_exit(1, "b\n");

	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE)
                port_conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE;

	if (rte_eth_dev_configure(port_id, 1, 1, &port_conf))
		rte_exit(1, "c\n");

	if (rte_eth_dev_adjust_nb_rx_tx_desc(port_id, &nb_rxd, &nb_txd))
		rte_exit(1, "d\n");

	if (rte_eth_rx_queue_setup(port_id, 0, nb_rxd, rte_eth_dev_socket_id(port_id), NULL, mbuf_pool) < 0)
		rte_exit(1, "e\n");

	txconf = dev_info.default_txconf;
        txconf.offloads = port_conf.txmode.offloads;
	if (rte_eth_tx_queue_setup(port_id, 0, nb_txd, rte_eth_dev_socket_id(port_id), &txconf) < 0)
		rte_exit(1, "f\n");

	if (rte_eth_dev_start(port_id) < 0)
		rte_exit(1, "g\n");

	printf("Calling rte_eth_dev_stop - will error without patch\n"); fflush(stdout);
	rte_eth_dev_stop(port_id);
	printf("Returned from rte_eth_dev_stop\n"); fflush(stdout);
	rte_eth_dev_close(port_id);

	rte_eal_cleanup();
}
=======================================

This problem is caused by tap_tx_intr_vec_uninstall() calling
rte_intr_instance_free() which frees pmd->intr_handle (and it doesn't
set pmd->intr_handle to NULL, although this is not the cause of the issue).

When tap_rx_intr_vec_uninstall() is called from tap_rx_intr_vec_set() with
the set parameter != 0, which occurs when rte_eth_dev_start() is called,
it frees pmd->intr_handle, and tap_rx_intr_vec_install() is subsequently
called. If intr_conf.rxq is not set, this does not cause an immediate
problem, but if it is set, it will write to (the now) unallocated memory.

The main problem occurs when tap_dev_stop() is called which in turn calls
tap_intr_handle_set() and tap_lsc_intr_handle_set(). This uses
pmd->intr_handle which has now been overwritten due to being previously
freed. When rte_intr_instance_free() is called via tap_rx_intr_vec_uninstall(),
due to intr_handle->alloc_flags having been overwritten by a subsequent
user of that memory, it enters the wrong block and calls free() rather
than rte_free(). This causes free() to call abort().


 Quentin Armitage (1):
   tap: fix write-after-free and double free of intr_handle

 drivers/net/tap/rte_eth_tap.c | 5 +++++
 drivers/net/tap/tap_intr.c    | 2 --
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/1] tap: fix write-after-free and double free of intr_handle

[Quentin Armitage]

rte_pmd_tun/tap_probe() allocates pmd->intr_handle in
eth_dev_tap_create() and it should not be freed until
rte_pmd_tap_remove() is called.

Inspection of tap_rx_intr_vec_set() shows that the call to
tap_tx_intr_vec_uninstall() was calling rte_intr_instance_free() but
tap_tx_intr_vec_install() can then be immediately called, and this then
uses pmd->intr_handle without it being reallocated.

This commit moves the call of rte_intr_instance_free() from
tap_tx_intr_vec_uninstall() to rte_pmd_tap_remove().

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 drivers/net/tap/rte_eth_tap.c | 5 +++++
 drivers/net/tap/tap_intr.c    | 2 --
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index bc3d56a311..aab1692ebf 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2612,14 +2612,19 @@ static int
 rte_pmd_tap_remove(struct rte_vdev_device *dev)
 {
 	struct rte_eth_dev *eth_dev = NULL;
+	struct pmd_internals *pmd;
+	struct rte_intr_handle *intr_handle;
 
 	/* find the ethdev entry */
 	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
 	if (!eth_dev)
 		return 0;
 
+	pmd = eth_dev->data->dev_private;
+	intr_handle = pmd->intr_handle;
 	tap_dev_close(eth_dev);
 	rte_eth_dev_release_port(eth_dev);
+	rte_intr_instance_free(intr_handle);
 
 	return 0;
 }
diff --git a/drivers/net/tap/tap_intr.c b/drivers/net/tap/tap_intr.c
index 56c343acea..a9097def1a 100644
--- a/drivers/net/tap/tap_intr.c
+++ b/drivers/net/tap/tap_intr.c
@@ -34,8 +34,6 @@ tap_rx_intr_vec_uninstall(struct rte_eth_dev *dev)
 	rte_intr_free_epoll_fd(intr_handle);
 	rte_intr_vec_list_free(intr_handle);
 	rte_intr_nb_efd_set(intr_handle, 0);
-
-	rte_intr_instance_free(intr_handle);
 }
 
 /**
-- 
2.34.1

Re: [PATCH 1/1] tap: fix write-after-free and double free of intr_handle

[David Marchand]

On Mon, May 2, 2022 at 9:23 PM Quentin Armitage <quentin@armitage.org.uk> wrote:
>
> rte_pmd_tun/tap_probe() allocates pmd->intr_handle in
> eth_dev_tap_create() and it should not be freed until
> rte_pmd_tap_remove() is called.
>
> Inspection of tap_rx_intr_vec_set() shows that the call to
> tap_tx_intr_vec_uninstall() was calling rte_intr_instance_free() but
> tap_tx_intr_vec_install() can then be immediately called, and this then
> uses pmd->intr_handle without it being reallocated.
>
> This commit moves the call of rte_intr_instance_free() from
> tap_tx_intr_vec_uninstall() to rte_pmd_tap_remove().

Good catch.
We want this fix backported:

Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")
Cc: stable@dpdk.org


>
> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> ---
>  drivers/net/tap/rte_eth_tap.c | 5 +++++
>  drivers/net/tap/tap_intr.c    | 2 --
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index bc3d56a311..aab1692ebf 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2612,14 +2612,19 @@ static int
>  rte_pmd_tap_remove(struct rte_vdev_device *dev)
>  {
>         struct rte_eth_dev *eth_dev = NULL;
> +       struct pmd_internals *pmd;
> +       struct rte_intr_handle *intr_handle;
>
>         /* find the ethdev entry */
>         eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(dev));
>         if (!eth_dev)
>                 return 0;
>
> +       pmd = eth_dev->data->dev_private;
> +       intr_handle = pmd->intr_handle;
>         tap_dev_close(eth_dev);
>         rte_eth_dev_release_port(eth_dev);
> +       rte_intr_instance_free(intr_handle);

intr_handle is shared for multiprocess, so freeing should happen in
the primary process only.
For this reason, rte_intr_instance_free() should probably go to tap_dev_close.


>
>         return 0;
>  }
> diff --git a/drivers/net/tap/tap_intr.c b/drivers/net/tap/tap_intr.c
> index 56c343acea..a9097def1a 100644
> --- a/drivers/net/tap/tap_intr.c
> +++ b/drivers/net/tap/tap_intr.c
> @@ -34,8 +34,6 @@ tap_rx_intr_vec_uninstall(struct rte_eth_dev *dev)
>         rte_intr_free_epoll_fd(intr_handle);
>         rte_intr_vec_list_free(intr_handle);
>         rte_intr_nb_efd_set(intr_handle, 0);
> -
> -       rte_intr_instance_free(intr_handle);
>  }
>
>  /**
> --
> 2.34.1
>


-- 
David Marchand