netdev - Re: [PATCH net 2/2] net/tls: Fix use-after-free after the TLS device goes down and up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210524091528.3b53c1ce@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 24 May 2021 09:15:28 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Maxim Mikityanskiy <maximmi@...dia.com>
Cc:     Boris Pismenny <borisp@...dia.com>,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        "Aviad Yehezkel" <aviadye@...dia.com>,
        Tariq Toukan <tariqt@...dia.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net 2/2] net/tls: Fix use-after-free after the TLS
 device goes down and up

On Mon, 24 May 2021 15:12:20 +0300 Maxim Mikityanskiy wrote:
> @@ -1290,6 +1304,26 @@ static int tls_device_down(struct net_device *netdev)
>  	spin_unlock_irqrestore(&tls_device_lock, flags);
>  
>  	list_for_each_entry_safe(ctx, tmp, &list, list)	{
> +		/* Stop offloaded TX and switch to the fallback.
> +		 * tls_is_sk_tx_device_offloaded will return false.
> +		 */
> +		WRITE_ONCE(ctx->sk->sk_validate_xmit_skb, tls_validate_xmit_skb_sw);
> +
> +		/* Stop the RX and TX resync.
> +		 * tls_dev_resync must not be called after tls_dev_del.
> +		 */
> +		WRITE_ONCE(ctx->netdev, NULL);
> +
> +		/* Start skipping the RX resync logic completely. */
> +		set_bit(TLS_RX_DEV_DEGRADED, &ctx->flags);
> +
> +		/* Sync with inflight packets. After this point:
> +		 * TX: no non-encrypted packets will be passed to the driver.
> +		 * RX: resync requests from the driver will be ignored.
> +		 */
> +		synchronize_net();
> +
> +		/* Release the offload context on the driver side. */
>  		if (ctx->tx_conf == TLS_HW)
>  			netdev->tlsdev_ops->tls_dev_del(netdev, ctx,
>  							TLS_OFFLOAD_CTX_DIR_TX);

Can we have the Rx resync take the device_offload_lock for read instead?
Like Tx already does?

> +EXPORT_SYMBOL_GPL(tls_validate_xmit_skb_sw);

Why the export?