Re: [PATCH v4] net: nfc: Fix use-after-free caused by nfc_llcp_find_local

[Simon Horman <simon.horman@corigine.com>]

+ Blamed authors; corected davem's email address

On Sun, Jun 25, 2023 at 08:54:39AM +0800, Lin Ma wrote:

...

> @@ -696,20 +696,22 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
>  	if (dev->dep_link_up == false) {
>  		ret = -ENOLINK;
>  		device_unlock(&dev->dev);
> -		goto put_dev;
> +		goto sock_llcp_put_local;
>  	}
>  	device_unlock(&dev->dev);
>  
>  	if (local->rf_mode == NFC_RF_INITIATOR &&
>  	    addr->target_idx != local->target_idx) {
>  		ret = -ENOLINK;
> -		goto put_dev;
> +		goto sock_llcp_put_local;
>  	}
>  
>  	llcp_sock->dev = dev;
> -	llcp_sock->local = nfc_llcp_local_get(local);
> +	llcp_sock->local = local;
>  	llcp_sock->ssap = nfc_llcp_get_local_ssap(local);
>  	if (llcp_sock->ssap == LLCP_SAP_MAX) {
> +		llcp_sock->local = NULL;
> +		llcp_sock->dev = NULL;
>  		ret = -ENOMEM;
>  		goto sock_llcp_put_local;
>  	}

Hi Lin Ma,

thanks for your patch.
I agree with the get/put handling in this patch.
But I have a question regarding the changes to llcp_sock_connect().

Setting of llcp_sock->local and llcp_sock->dev has been moved
from the sock_llcp_put_local label, below,
to the error handling for nfc_llcp_get_local_ssap, above.

For the case where nfc_llcp_get_local_ssap returns an error this seems
fine.  However, there are two other cases that this effects. Relevant
code, which appears between the hunk above and the one below is:

--- start ---
        llcp_sock->service_name = kmemdup(addr->service_name,
                                          llcp_sock->service_name_len,
                                          GFP_KERNEL);
        if (!llcp_sock->service_name) {
                ret = -ENOMEM;
                goto sock_llcp_release;
        }

        nfc_llcp_sock_link(&local->connecting_sockets, sk);

        ret = nfc_llcp_send_connect(llcp_sock);
        if (ret)
                goto sock_unlink;

        sk->sk_state = LLCP_CONNECTING;

        ret = sock_wait_state(sk, LLCP_CONNECTED,
                              sock_sndtimeo(sk, flags & O_NONBLOCK));
        if (ret && ret != -EINPROGRESS)
                goto sock_unlink;
--- end ---

It seems to me that in the error handling above,
where the gotos will feed into sock_llcp_put_local,
llcp_sock->local and llcp_sock->dev will not be reset to NULL
in these cases anymore.

Is that ok?

> @@ -758,9 +760,7 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
>  	nfc_llcp_put_ssap(local, llcp_sock->ssap);
>  
>  sock_llcp_put_local:
> -	nfc_llcp_local_put(llcp_sock->local);
> -	llcp_sock->local = NULL;
> -	llcp_sock->dev = NULL;
> +	nfc_llcp_local_put(local);
>  
>  put_dev:
>  	nfc_put_device(dev);

...