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

Bjorn Helgaas
2021-04-22T04:44:06Z

[+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@umn.edu>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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
>