netdev - Re: [PATCH net] nfc: avoid potential race condition

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: <3760c70c-299c-89bf-5a4a-22e8d564ef92@canonical.com>
Date:   Thu, 23 Sep 2021 09:26:51 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Samuel Ortiz <sameo@...ux.intel.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "John W. Linville" <linville@...driver.com>,
        netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net] nfc: avoid potential race condition

On 23/09/2021 08:50, Dan Carpenter wrote:
> This from static analysis inspired by CVE-2021-26708 where there was a
> race condition because it didn't lock_sock(sk) before saving
> "vsk->transport".  Here it is saving "llcp_sock->local" but the concept
> is the same that it needs to take the lock first.

I think the difference between this llcp_sock code and above transport,
is lack of writer to llcp_sock->local with whom you could race.

Commits c0cfa2d8a788fcf4 and 6a2c0962105ae8ce causing the
multi-transport race show nicely assigns to vsk->transport when module
is unloaded.

Here however there is no writer to llcp_sock->local, except bind and
connect and their error paths. The readers which you modify here, have
to happen after bind/connect. You cannot have getsockopt() or release()
before bind/connect, can you? Unless you mean here the bind error path,
where someone calls getsockopt() in the middle of bind()? Is it even
possible?

The code except this looks reasonable and since writer protects
llcp_sock->local(), the reader I guess should do it as well... just
wondering whether this is a real issue.


Best regards,
Krzysztof

> 
> Fixes: 00e856db49bb ("NFC: llcp: Fall back to local values when getting socket options")
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
>  net/nfc/llcp_sock.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 6cfd30fc0798..74f4209c7144 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -314,14 +314,16 @@ static int nfc_llcp_getsockopt(struct socket *sock, int level, int optname,
>  	if (get_user(len, optlen))
>  		return -EFAULT;
>  
> -	local = llcp_sock->local;
> -	if (!local)
> -		return -ENODEV;
> -
>  	len = min_t(u32, len, sizeof(u32));
>  
>  	lock_sock(sk);
>  
> +	local = llcp_sock->local;
> +	if (!local) {
> +		release_sock(sk);
> +		return -ENODEV;
> +	}
> +
>  	switch (optname) {
>  	case NFC_LLCP_RW:
>  		rw = llcp_sock->rw > LLCP_MAX_RW ? local->rw : llcp_sock->rw;
> @@ -598,14 +600,15 @@ static int llcp_sock_release(struct socket *sock)
>  
>  	pr_debug("%p\n", sk);
>  
> +	lock_sock(sk);
> +
>  	local = llcp_sock->local;
>  	if (local == NULL) {
> +		release_sock(sk);
>  		err = -ENODEV;
>  		goto out;
>  	}
>  
> -	lock_sock(sk);
> -
>  	/* Send a DISC */
>  	if (sk->sk_state == LLCP_CONNECTED)
>  		nfc_llcp_send_disconnect(llcp_sock);
>