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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <475fb577-a8cc-d1dc-e522-13333c7975a2@chelsio.com>
Date:   Thu, 28 May 2020 09:36:43 +0530
From:   rohit maheshwari <rohitm@...lsio.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 28/05/20 2:34 AM, Jakub Kicinski wrote:
> 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.
CHCR driver is a ULD driver, and if user requests to remove chcr alone,
this cleanup will be done. This is why I am taking module refcount until
tls offload flag is set, or any single tls offload connection exists.  
So, now,
when this cleanup will be triggered, TLS offload won't be enabled, and
this crash situation can never occur.