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  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]
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.

Powered by blists - more mailing lists