netdev - Re: [PATCH] net: nfc: Fix use-after-free in local

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

Message-ID: <0a0932848b04b844d94561c3dc4186b6ae9dbe31.camel@redhat.com>
Date:   Thu, 05 Jan 2023 12:41:31 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jisoo Jang <jisoo.jang@...sei.ac.kr>,
        krzysztof.kozlowski@...aro.org, netdev@...r.kernel.org
Cc:     edumazet@...gle.com, kuba@...nel.org, dokyungs@...sei.ac.kr,
        linuxlovemin@...sei.ac.kr
Subject: Re: [PATCH] net: nfc: Fix use-after-free in local_cleanup()

On Wed, 2023-01-04 at 21:57 +0900, Jisoo Jang wrote:
> Fix a use-after-free that occurs in kfree_skb() called from
> local_cleanup(). When detaching an nfc device, local_cleanup()
> called from nfc_llcp_unregister_device() frees local->rx_pending
> and cancels local->rx_work. So the socket allocated before
> unregister is not set null by nfc_llcp_rx_work().
> local_cleanup() called from local_release() frees local->rx_pending
> again, which leads to the bug.
> 
> Ensure kfree_skb() is called once when unregistering the device.
> 
> Found by a modified version of syzkaller.
> 
> BUG: KASAN: use-after-free in kfree_skb
> Call Trace:
>  kfree_skb
>  local_cleanup
>  nfc_llcp_local_put
>  llcp_sock_destruct
>  __sk_destruct
>  sk_destruct
>  __sk_free
>  sk_free
>  llcp_sock_release
>  __sock_release
>  sock_close
>  __fput
>  task_work_run
>  exit_to_user_mode_prepare
>  syscall_exit_to_user_mode
>  do_syscall_64
>  entry_SYSCALL_64_after_hwframe
> 
> Allocate by:
>  __alloc_skb
>  pn533_recv_response
>  __usb_hcd_giveback_urb
>  usb_hcd_giveback_urb
>  dummy_timer
>  call_timer_fn
>  run_timer_softirq
>  __do_softirq
> 
> Freed by:
>  kfree_skbmem
>  kfree_skb
>  local_cleanup
>  nfc_llcp_unregister_device
>  nfc_unregister_device
>  pn53x_unregister_nfc
>  pn533_usb_disconnect
>  usb_unbind_interface
>  device_release_driver_internal
>  bus_remove_device
>  device_del
>  usb_disable_device
>  usb_disconnect
>  hub_event
>  process_one_work
>  worker_thread
>  kthread
>  ret_from_fork
> 
> Signed-off-by: Jisoo Jang <jisoo.jang@...sei.ac.kr>
> ---
>  net/nfc/llcp_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 3364caabef8b..cbf2ef0af57b 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -156,7 +156,6 @@ static void local_cleanup(struct nfc_llcp_local *local)
>  	cancel_work_sync(&local->tx_work);
>  	cancel_work_sync(&local->rx_work);
>  	cancel_work_sync(&local->timeout_work);
> -	kfree_skb(local->rx_pending);
>  	del_timer_sync(&local->sdreq_timer);
>  	cancel_work_sync(&local->sdreq_timeout_work);
>  	nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
> @@ -170,6 +169,7 @@ static void local_release(struct kref *ref)
>  
>  	list_del(&local->list);
>  	local_cleanup(local);
> +	kfree_skb(local->rx_pending);
>  	kfree(local);
>  }
> 

With this change, local->tx_work/nfc_llcp_tx_work can be invoked after
nfc_unregister_device(). nfc_llcp_tx_work assumnes the nfc device is
alive, so I guess we need (part of) local_cleanup() being in place at
nfc de-registration time.

What about simply setting local->rx_pending to NULL in local_cleanup()?

Thanks!

Paolo