netdev - Re: [net] ch_ktls: lock is not freed

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] [day] [month] [year] [list]

Message-ID: <20201120103607.58074173@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 20 Nov 2020 10:36:07 -0800
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: [net] ch_ktls: lock is not freed

On Wed, 18 Nov 2020 13:51:07 +0530 Rohit Maheshwari wrote:
> Currently lock gets freed only if timeout expires, but missed a
> case when HW returns failure and goes for cleanup.
> 
> Fixes: efca3878a5fb ("ch_ktls: Issue if connection offload fails")
> Signed-off-by: Rohit Maheshwari <rohitm@...lsio.com>
> ---
>  .../net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c   | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> index c24485c0d512..1f521751666d 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
> @@ -594,9 +594,10 @@ static int chcr_ktls_dev_add(struct net_device *netdev, struct sock *sk,
>  free_l2t:
>  	cxgb4_l2t_release(tx_info->l2te);
>  free_tx_info:
> -	if (tx_info->pending_close)
> +	if (tx_info->open_state)
>  		spin_unlock_bh(&tx_info->lock);
> -	else
> +
> +	if (!tx_info->pending_close)
>  		kvfree(tx_info);
>  out:
>  	atomic64_inc(&port_stats->ktls_tx_connection_fail);

Are you 100% sure about this fix? The code seems to be jumping to the
error handler with or without this lock held. E.g. on line 558.

Please release the lock before jumping, in the two places that hold it.
It's far less complicated and actually fewer LoC.