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 13:03:30 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Vishnu Dasa <vdasa@...are.com>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        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 Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
> 
> > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> > 
> > Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
> >> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:

> >> Whatever...  But where this really
> >> hurts is with:
> >> Alloc:
> >> 	if (vmci_dev->exclusive_vectors) {
> >> 		error = request_irq(pci_irq_vector(pdev, 1), ...
> >> Free:
> >> 	free_irq(pci_irq_vector(pdev, 1), vmci_dev);
> >> No if statement.  It works because it's the last allocation but it's
> >> confusing and fragile.
> > 
> > Agreed.
> 
> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
> But I don't think the problem implied here exists?
> 
> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
> is no free_irq in this path.  If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'.  Note that
> this is for the previous one without the check for vmci_dev->exclusive_vectors.

It's not a bug, but it's bad style.

Ideally the allocation code and the free code should mirror each other
as much as possible.  If there is an if statement in the allocation code
we should copy that same if statement into the free code.  Even if we
can leave the if statement out, we should still include it for
readability.

Also if we add another allocation at the end of the function then we
will have to add the if statement back.  Maybe the person who adds the
if statement will forget.  So it's fragile.

regards,
dan carpenter