linux-kernel - Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe

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

Date:   Thu, 24 Feb 2022 00:43:26 +0000
From:   Vishnu Dasa <vdasa@...are.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     Jorgen Hansen <jhansen@...are.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "acking@...are.com" <acking@...are.com>,
        "dtor@...are.com" <dtor@...are.com>,
        Pv-drivers <Pv-drivers@...are.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH 3/3] VMCI: Fix some error handling paths in
 vmci_guest_probe_device()



> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> 
> The 'err_remove_vmci_dev_g' error label is not at the right place.
> This could lead to un-released resource.
> 
> There is also a missing label. If pci_alloc_irq_vectors() fails, the
> previous vmci_event_subscribe() call must be undone.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>

Thanks for the patch!
Let's deal with the points which Dan brought up separately.

Acked-by: Vishnu Dasa <vdasa@...are.com>

> ---
> Review with GREAT care.
> 
> This patch is a recent rebase of an old patch that has never been
> submitted.
> This function is huge and modifying its error handling path is error
> prone (at least for me).
> 
> The patch is compile-tested only.
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index 02d4722d8474..2e8ad36895ae 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 	/* Check host capabilities. */
> 	error = vmci_check_host_caps(pdev);
> 	if (error)
> -		goto err_remove_bitmap;
> +		goto err_remove_vmci_dev_g;
> 
> 	/* Enable device. */
> 
> @@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 		error = pci_alloc_irq_vectors(pdev, 1, 1,
> 				PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
> 		if (error < 0)
> -			goto err_remove_bitmap;
> +			goto err_unsubscrive_event;
> 	} else {
> 		vmci_dev->exclusive_vectors = true;
> 	}
> @@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> err_disable_msi:
> 	pci_free_irq_vectors(pdev);
> 
> +err_unsubscrive_event:

Please rename this to err_unsubscribe_event

> 	vmci_err = vmci_event_unsubscribe(ctx_update_sub_id);
> 	if (vmci_err < VMCI_SUCCESS)
> 		dev_warn(&pdev->dev,
> 			 "Failed to unsubscribe from event (type=%d) with subscriber (ID=0x%x): %d\n",
> 			 VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
> 
> -err_remove_bitmap:
> +err_remove_vmci_dev_g:
> +	spin_lock_irq(&vmci_dev_spinlock);
> +	vmci_pdev = NULL;
> +	vmci_dev_g = NULL;
> +	spin_unlock_irq(&vmci_dev_spinlock);
> +
> 	if (vmci_dev->notification_bitmap) {
> 		vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
> 		dma_free_coherent(&pdev->dev, PAGE_SIZE,
> @@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> 				  vmci_dev->notification_base);
> 	}
> 
> -err_remove_vmci_dev_g:
> -	spin_lock_irq(&vmci_dev_spinlock);
> -	vmci_pdev = NULL;
> -	vmci_dev_g = NULL;
> -	spin_unlock_irq(&vmci_dev_spinlock);
> -
> err_free_data_buffers:
> 	vmci_free_dg_buffers(vmci_dev);
> 
> -- 
> 2.32.0
>