netdev - Re: [PATCH net v2] cxgb4/chcr: Enable ktls settings at run time

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200527140406.420ed7fd@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Wed, 27 May 2020 14:04:06 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     rohit maheshwari <rohitm@...lsio.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, secdev@...lsio.com
Subject: Re: [PATCH net v2] cxgb4/chcr: Enable ktls settings at run time

On Wed, 27 May 2020 10:02:42 +0530 rohit maheshwari wrote:
> On 27/05/20 4:12 AM, Jakub Kicinski wrote:
> > On Tue, 26 May 2020 19:36:34 +0530 Rohit Maheshwari wrote:  
> >> Current design enables ktls setting from start, which is not
> >> efficient. Now the feature will be enabled when user demands
> >> TLS offload on any interface.
> >>
> >> v1->v2:
> >> - taking ULD module refcount till any single connection exists.
> >> - taking rtnl_lock() before clearing tls_devops.  
> > Callers of tls_devops don't hold the rtnl_lock.  
> I think I should correct the statement here, " taking rtnl_lock()
> before clearing tls_devops and device flags". There won't be any
> synchronization issue while clearing tls_devops now, because I
> am incrementing module refcount of CRYPTO ULD, so this will
> never be called if there is any connection (new connection
> request) exists.

Please take a look at tls_set_device_offload():

	if (!(netdev->features & NETIF_F_HW_TLS_TX)) {
		rc = -EOPNOTSUPP;
		goto release_netdev;
	}

	/* Avoid offloading if the device is down
	 * We don't want to offload new flows after
	 * the NETDEV_DOWN event
	 *
	 * device_offload_lock is taken in tls_devices's NETDEV_DOWN
	 * handler thus protecting from the device going down before
	 * ctx was added to tls_device_list.
	 */
	down_read(&device_offload_lock);
	if (!(netdev->flags & IFF_UP)) {
		rc = -EINVAL;
		goto release_lock;
	}

	ctx->priv_ctx_tx = offload_ctx;
	rc = netdev->tlsdev_ops->tls_dev_add(netdev, sk, TLS_OFFLOAD_CTX_DIR_TX,
					     &ctx->crypto_send.info,
					     tcp_sk(sk)->write_seq);

This does not hold rtnl_lock. If you clear the ops between the feature
check and the call - there will be a crash. Never clear tls ops on a
registered netdev.

Why do you clear the ops in the first place? It shouldn't be necessary.