linux-kernel - Re: [PATCH] hci: fix double free in hci_req

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: <4036b194-3014-4523-b8d0-ea39664607a3@web.de>
Date: Tue, 2 Jul 2024 21:40:21 +0200
From: Markus Elfring <Markus.Elfring@....de>
To: Levi Yun <yeoreum.yun@....com>, Yunseong Kim <yskelg@...il.com>,
 linux-bluetooth@...r.kernel.org, Johan Hedberg <johan.hedberg@...il.com>,
 Luiz Von Dentz <luiz.dentz@...il.com>, Marcel Holtmann <marcel@...tmann.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Austin Kim <austindh.kim@...il.com>,
 MichelleJin <shjy180909@...il.com>, Levi Yun <ppbuk5246@...il.com>
Subject: Re: [PATCH] hci: fix double free in hci_req_sync

…
> +++ b/net/bluetooth/hci_request.c
> @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode,
>  		hdev->req_result = result;
>  		hdev->req_status = HCI_REQ_DONE;
>  		if (skb) {
> -			kfree_skb(hdev->req_skb);
> -			hdev->req_skb = skb_get(skb);
> +			hci_req_skb_release_and_set(hdev, skb_get(skb));
>  		}
>  		wake_up_interruptible(&hdev->req_wait_q);
…

How do you think about to omit any curly brackets here?


…
> +++ b/net/bluetooth/hci_request.h
> @@ -28,6 +28,15 @@
>
>  #define hci_req_sync_lock(hdev)   mutex_lock(&hdev->req_lock)
>  #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock)
> +#define hci_req_skb_release_and_set(hdev, val)		\
> +({							\
> +	if (hdev->req_skb) {				\
> +		spin_lock(&hdev->req_skb_lock);		\
> +		kfree_skb(hdev->req_skb);		\
> +		hdev->req_skb = val;			\
> +		spin_unlock(&hdev->req_skb_lock);	\
> +	}						\
> +})
…

* Do you expect that any data synchronisation should be performed
  for the shown pointer check?

* Can it eventually matter to implement such a macro with a statement
  like “guard(spinlock)(&hdev->req_skb_lock);”?
  https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/spinlock.h#L561


Regards,
Markus