netdev - RE: [PATCH net-next v3] nfc: Allow to create multiple virtual nci devices

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

Date:   Mon, 07 Nov 2022 11:46:04 +0900
From:   Bongsu Jeon <bongsu.jeon@...sung.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>,
        "leon@...nel.org" <leon@...nel.org>,
        Bongsu Jeon <bongsu.jeon@...sung.com>,
        "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "syzkaller@...glegroups.com" <syzkaller@...glegroups.com>
Subject: RE: [PATCH net-next v3] nfc: Allow to create multiple virtual nci
 devices

On Sat, Nov 5, 2022 at 2:04 AM Dmitry Vyukov<dvyukov@...gle.com> wrote:
> The current virtual nci driver is great for testing and fuzzing.
> But it allows to create at most one "global" device which does not allow
> to run parallel tests and harms fuzzing isolation and reproducibility.
> Restructure the driver to allow creation of multiple independent devices.
> This should be backwards compatible for existing tests.

I totally agree with you for parallel tests and good design.
Thanks for good idea.
But please check the abnormal situation.
for example virtual device app is closed(virtual_ncidev_close) first and then
virtual nci driver from nci app tries to call virtual_nci_send or virtual_nci_close.
(there would be problem in virtual_nci_send because of already destroyed mutex)
Before this patch, this driver used virtual_ncidev_mode state and nci_mutex that isn't destroyed.

> 
> Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> Cc: Bongsu Jeon <bongsu.jeon@...sung.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Cc: netdev@...r.kernel.org
> 
> ---
> Changes in v3:
>  - free vdev in virtual_ncidev_close()
> 
> Changes in v2:
>  - check return value of skb_clone()
>  - rebase onto currnet net-next
> ---
>  drivers/nfc/virtual_ncidev.c | 147 +++++++++++++++++------------------
>  1 file changed, 71 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c
> index 85c06dbb2c449..bb76c7c7cc822 100644
> --- a/drivers/nfc/virtual_ncidev.c
> +++ b/drivers/nfc/virtual_ncidev.c
> @@ -13,12 +13,6 @@
>  
>  static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
>  {
> -	mutex_lock(&nci_mutex);
> -	if (state != virtual_ncidev_enabled) {
> -		mutex_unlock(&nci_mutex);
> +	struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
> +
> +	mutex_lock(&vdev->mtx);
  
  I think this vdev and vdev->mtx are already destroyed so that it would be problem.

> +	if (vdev->send_buff) {
> +		mutex_unlock(&vdev->mtx);
>  		kfree_skb(skb);
> -		return 0;
> +		return -1;
>  	}
> 
> 	
>  static int virtual_ncidev_close(struct inode *inode, struct file *file)
>  {
> -	mutex_lock(&nci_mutex);
> -
> -	if (state == virtual_ncidev_enabled) {
> -		state = virtual_ncidev_disabling;
> -		mutex_unlock(&nci_mutex);
> +	struct virtual_nci_dev *vdev = file->private_data;
>  
> -		nci_unregister_device(ndev);
> -		nci_free_device(ndev);
> -
> -		mutex_lock(&nci_mutex);
> -	}
> -
> -	state = virtual_ncidev_disabled;
> -	mutex_unlock(&nci_mutex);
> +	nci_unregister_device(vdev->ndev);
> +	nci_free_device(vdev->ndev);
> +	mutex_destroy(&vdev->mtx);
> +	kfree(vdev);
>  
>  	return 0;
>  }