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

Date:   Thu, 24 Feb 2022 16:08:25 +0000
From:   Vishnu Dasa <vdasa@...are.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     Jorgen Hansen <jhansen@...are.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "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 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> 
> 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.

No disagreements about that.  Making sure I wasn't missing anything
else.

Could we have a separate fix for this which fixes?
"VMCI: dma dg: register dummy IRQ handlers for DMA datagrams"

Thanks!