Re: [PATCH 020/190] Revert "PCI: Fix pci_create_slot() reference count leak"

[Bjorn Helgaas]

[+cc Jiri, Jubin (author of 4684709bf81a)]

On Wed, Apr 21, 2021 at 02:58:15PM +0200, Greg Kroah-Hartman wrote:
> This reverts commit 8a94644b440eef5a7b9c104ac8aa7a7f413e35e5.
> 
> Commits from @umn.edu addresses have been found to be submitted in "bad
> faith" to try to test the kernel community's ability to review "known
> malicious" changes.  The result of these submissions can be found in a
> paper published at the 42nd IEEE Symposium on Security and Privacy
> entitled, "Open Source Insecurity: Stealthily Introducing
> Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
> of Minnesota) and Kangjie Lu (University of Minnesota).
> 
> Because of this, all submissions from this group must be reverted from
> the kernel tree and will need to be re-reviewed again to determine if
> they actually are a valid fix.  Until that work is complete, remove this
> change to ensure that no problems are being introduced into the
> codebase.
> 
> Cc: https
> Cc: Qiushi Wu <wu000273@xxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

Please do not apply this revert.

Prior to 8a94644b440e ("PCI: Fix pci_create_slot() reference count
leak"), we essentially had this:

  err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, ...);
  if (err)
    kfree(slot);
    return ERR_PTR(err);

  INIT_LIST_HEAD(&slot->list);
  list_add(&slot->list, &parent->slots);

That was incorrect because if kobject_init_and_add() fails,
kobject_put() must be called to clean up the object (per the function
comment).  For pci_slot_ktype, the release function is
pci_slot_release():

  pci_slot_release
    list_del(&slot->list);
    kfree(slot);

After 8a94644b440e, we had:

  err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, ...);
  if (err)
    kobject_put(&slot->kobj);
    return ERR_PTR(err);

  INIT_LIST_HEAD(&slot->list);
  list_add(&slot->list, &parent->slots);

This fixed one bug but exposed another: we correctly clean up the
object by calling kobject_put() which calls pci_slot_release(), but 
that dereferences slot->list, which hasn't been initialized yet.

But 4684709bf81a ("PCI: Fix pci_slot_release() NULL pointer
dereference") fixed that problem by making it this:

  INIT_LIST_HEAD(&slot->list);
  list_add(&slot->list, &parent->slots);
  err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, ...);
  if (err)
    kobject_put(&slot->kobj);
    return ERR_PTR(err);

This correctly initializes slot->list and cleans up if
kobject_init_and_add() fails.

But if we apply this revert, we'll have this:

  INIT_LIST_HEAD(&slot->list);
  list_add(&slot->list, &parent->slots);
  err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, ...);
  if (err)
    kfree(slot);
    return ERR_PTR(err);

Now we kfree(slot), but we don't call kobject_put(), so we don't
remove it from the list, so the list is now corrupted because one of
its entries has been deallocated.

Bjorn

> ---
>  drivers/pci/slot.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index d627dd9179b4..c190e09af356 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -268,7 +268,6 @@ 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;
>  	}
>  
> @@ -277,10 +276,8 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  
>  	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
>  				   "%s", slot_name);
> -	if (err) {
> -		kobject_put(&slot->kobj);
> +	if (err)
>  		goto err;
> -	}
>  
>  	down_read(&pci_bus_sem);
>  	list_for_each_entry(dev, &parent->devices, bus_list)
> @@ -296,6 +293,7 @@ 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.31.1
>