Re: [PATCH RESEND] Bluetooth: vhci: Prevent use-after-free by removing debugfs files early

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Ivan,


Thank you for your patch.

Am 16.07.25 um 02:42 schrieb Ivan Pravdin:
> Move the creation of debugfs files into a dedicated function, and ensure
> they are explicitly removed during vhci_release(), before associated
> data structures are freed.
> 
> Previously, debugfs files such as "force_suspend", "force_wakeup", and
> others were created under hdev->debugfs but not removed in
> vhci_release(). Since vhci_release() frees the backing vhci_data
> structure, any access to these files after release would result in
> use-after-free errors.
> 
> Although hdev->debugfs is later freed in hci_release_dev(), user can
> access files after vhci_data is freed but before hdev->debugfs is
> released.
> 
> Signed-off-by: Ivan Pravdin <ipravdin.official@gmail.com>
> Fixes: ab4e4380d4e1 ("Bluetooth: Add vhci devcoredump support")

Not important, but I’d put the Signed-off-by: tag last.

> ---
> Resending because previous patch got archived [1].
> 
> [1] https://patchwork.kernel.org/project/bluetooth/patch/20250614235317.304705-1-ipravdin.official@gmail.com/
> 
>   drivers/bluetooth/hci_vhci.c | 57 ++++++++++++++++++++++++++----------
>   1 file changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> index f7d8c3c00655..2fef08254d78 100644
> --- a/drivers/bluetooth/hci_vhci.c
> +++ b/drivers/bluetooth/hci_vhci.c
> @@ -380,6 +380,28 @@ static const struct file_operations force_devcoredump_fops = {
>   	.write		= force_devcd_write,
>   };
>   
> +static void vhci_debugfs_init(struct vhci_data *data)
> +{
> +	struct hci_dev *hdev = data->hdev;
> +
> +	debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
> +			    &force_suspend_fops);
> +
> +	debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
> +			    &force_wakeup_fops);
> +
> +	if (IS_ENABLED(CONFIG_BT_MSFTEXT))
> +		debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
> +				    &msft_opcode_fops);
> +
> +	if (IS_ENABLED(CONFIG_BT_AOSPEXT))
> +		debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
> +				    &aosp_capable_fops);
> +
> +	debugfs_create_file("force_devcoredump", 0644, hdev->debugfs, data,
> +			    &force_devcoredump_fops);
> +}
> +
>   static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
>   {
>   	struct hci_dev *hdev;
> @@ -434,22 +456,8 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
>   		return -EBUSY;
>   	}
>   
> -	debugfs_create_file("force_suspend", 0644, hdev->debugfs, data,
> -			    &force_suspend_fops);
> -
> -	debugfs_create_file("force_wakeup", 0644, hdev->debugfs, data,
> -			    &force_wakeup_fops);
> -
> -	if (IS_ENABLED(CONFIG_BT_MSFTEXT))
> -		debugfs_create_file("msft_opcode", 0644, hdev->debugfs, data,
> -				    &msft_opcode_fops);
> -
> -	if (IS_ENABLED(CONFIG_BT_AOSPEXT))
> -		debugfs_create_file("aosp_capable", 0644, hdev->debugfs, data,
> -				    &aosp_capable_fops);
> -
> -	debugfs_create_file("force_devcoredump", 0644, hdev->debugfs, data,
> -			    &force_devcoredump_fops);
> +	if (!IS_ERR_OR_NULL(hdev->debugfs))
> +		vhci_debugfs_init(data);
>   
>   	hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
>   
> @@ -651,6 +659,21 @@ static int vhci_open(struct inode *inode, struct file *file)
>   	return 0;
>   }
>   
> +static void vhci_debugfs_remove(struct hci_dev *hdev)
> +{
> +	debugfs_lookup_and_remove("force_suspend", hdev->debugfs);
> +
> +	debugfs_lookup_and_remove("force_wakeup", hdev->debugfs);
> +
> +	if (IS_ENABLED(CONFIG_BT_MSFTEXT))
> +		debugfs_lookup_and_remove("msft_opcode", hdev->debugfs);
> +
> +	if (IS_ENABLED(CONFIG_BT_AOSPEXT))
> +		debugfs_lookup_and_remove("aosp_capable", hdev->debugfs);
> +
> +	debugfs_lookup_and_remove("force_devcoredump", hdev->debugfs);
> +}
> +
>   static int vhci_release(struct inode *inode, struct file *file)
>   {
>   	struct vhci_data *data = file->private_data;
> @@ -662,6 +685,8 @@ static int vhci_release(struct inode *inode, struct file *file)
>   	hdev = data->hdev;
>   
>   	if (hdev) {
> +		if (!IS_ERR_OR_NULL(hdev->debugfs))
> +			vhci_debugfs_remove(hdev);
>   		hci_unregister_dev(hdev);
>   		hci_free_dev(hdev);
>   	}

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul