linux-kernel - Re: [PATCH] Fix reference count leak in pci_create

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:   Fri, 10 Jul 2020 16:02:35 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     wu000273@....edu
Cc:     kjlu@....edu, Bjorn Helgaas <bhelgaas@...gle.com>,
        Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
        Jesse Barnes <jbarnes@...tuousgeek.org>,
        Alex Chiang <achiang@...com>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix reference count leak in pci_create_slot

On Wed, May 27, 2020 at 09:13:22PM -0500, wu000273@....edu wrote:
> From: Qiushi Wu <wu000273@....edu>
> 
> kobject_init_and_add() takes reference even when it fails.
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object. Thus,
> when call of kobject_init_and_add() fail, we should call kobject_put()
> instead of kfree(). Previous commit "b8eb718348b8" fixed a similar problem.

Looks like you are also checking other kobject_init_and_add() callers?
I see some similar messages on linux-kernel.  Thanks for doing that!

I looked at the first dozen or so callers and the following look
broken and I don't see patches for them (yet):

  cache_add_dev()
  sq_dev_add
  blk_integrity_add
  acpi_expose_nondev_subnodes
  rnbd_clt_add_dev_kobj

> Fixes: 5fe6cc60680d ("PCI: prevent duplicate slot names")

I'm not sure this is correct.  5fe6cc60680d didn't *add* a
kobject_init_and_add() call; it was there before.  And even before
5fe6cc60680d there was no kobject_put() in the error path.  Am I
missing something?

For now, I applied this to pci/hotplug for v5.9.  I'll update the
commit log to add the Fixes tag if necessary.

> Signed-off-by: Qiushi Wu <wu000273@....edu>
> ---
>  drivers/pci/slot.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index cc386ef2fa12..3861505741e6 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -268,13 +268,16 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  	slot_name = make_slot_name(name);
>  	if (!slot_name) {
>  		err = -ENOMEM;
> +		kfree(slot);
>  		goto err;
>  	}
>  
>  	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
>  				   "%s", slot_name);
> -	if (err)
> +	if (err) {
> +		kobject_put(&slot->kobj);
>  		goto err;
> +	}
>  
>  	INIT_LIST_HEAD(&slot->list);
>  	list_add(&slot->list, &parent->slots);
> @@ -293,7 +296,6 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  	mutex_unlock(&pci_slot_mutex);
>  	return slot;
>  err:
> -	kfree(slot);
>  	slot = ERR_PTR(err);
>  	goto out;
>  }
> -- 
> 2.17.1
>