[PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()

[PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()

[Jubin Zhong]

Once kobject_init_and_add() failed, pci_slot_release() is called to
delete slot->list from parent->slots. But slot->list is intialized
afterwards, so we ran into the following crash:

  Unable to handle kernel NULL pointer dereference at virtual address
00000000
  ...
  CPU: 10 PID: 1 Comm: swapper/0 Not tainted 4.4.240 #197
  task: ffffeb398a45ef10 task.stack: ffffeb398a470000
  PC is at __list_del_entry_valid+0x5c/0xb0
  LR is at pci_slot_release+0x84/0xe4
  ...
  __list_del_entry_valid+0x5c/0xb0
  pci_slot_release+0x84/0xe4
  kobject_put+0x184/0x1c4
  pci_create_slot+0x17c/0x1b4
  __pci_hp_initialize+0x68/0xa4
  pciehp_probe+0x1a4/0x2fc
  pcie_port_probe_service+0x58/0x84
  driver_probe_device+0x320/0x470
  __driver_attach+0x54/0xb8
  bus_for_each_dev+0xc8/0xf0
  driver_attach+0x30/0x3c
  bus_add_driver+0x1b0/0x24c
  driver_register+0x9c/0xe0
  pcie_port_service_register+0x64/0x7c
  pcied_init+0x44/0xa4
  do_one_initcall+0x1d0/0x1f0
  kernel_init_freeable+0x24c/0x254
  kernel_init+0x18/0xe8
  ret_from_fork+0x10/0x20

Fixes: 8a94644b440e ("PCI: Fix pci_create_slot() reference count leak")
Signed-off-by: Jubin Zhong <zhongjubin@huawei.com>
Cc: stable@vger.kernel.org #v4.4.235

----
v2:
  Since both slot memory and slot->list would be handled by
pci_slot_release(), we need to make sure slot->list is properly
initialized beforehand.

v1: https://lore.kernel.org/lkml/1606288971-47927-1-git-send-email-zhongjubin@huawei.com/
  Two things need to be cleaned up on pci_create_slot's error path:
  1. free slot memory
  2. remove slot->list from its parent->slots
  This patch mistakenly took slot memory as unfreed (which is not), and
would introduce double free problem.
---
 drivers/pci/slot.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 3861505..ed2077e 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -272,6 +272,9 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 		goto err;
 	}
 
+	INIT_LIST_HEAD(&slot->list);
+	list_add(&slot->list, &parent->slots);
+
 	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
 				   "%s", slot_name);
 	if (err) {
@@ -279,9 +282,6 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 		goto err;
 	}
 
-	INIT_LIST_HEAD(&slot->list);
-	list_add(&slot->list, &parent->slots);
-
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &parent->devices, bus_list)
 		if (PCI_SLOT(dev->devfn) == slot_nr)
-- 
1.8.5.6

Re: [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()

[Bjorn Helgaas]

On Wed, Dec 02, 2020 at 10:33:42AM +0800, Jubin Zhong wrote:
> Once kobject_init_and_add() failed, pci_slot_release() is called to
> delete slot->list from parent->slots. But slot->list is intialized
> afterwards, so we ran into the following crash:
> 
>   Unable to handle kernel NULL pointer dereference at virtual address
> 00000000
>   ...
>   CPU: 10 PID: 1 Comm: swapper/0 Not tainted 4.4.240 #197
>   task: ffffeb398a45ef10 task.stack: ffffeb398a470000
>   PC is at __list_del_entry_valid+0x5c/0xb0
>   LR is at pci_slot_release+0x84/0xe4
>   ...
>   __list_del_entry_valid+0x5c/0xb0
>   pci_slot_release+0x84/0xe4
>   kobject_put+0x184/0x1c4
>   pci_create_slot+0x17c/0x1b4
>   __pci_hp_initialize+0x68/0xa4
>   pciehp_probe+0x1a4/0x2fc
>   pcie_port_probe_service+0x58/0x84
>   driver_probe_device+0x320/0x470
>   __driver_attach+0x54/0xb8
>   bus_for_each_dev+0xc8/0xf0
>   driver_attach+0x30/0x3c
>   bus_add_driver+0x1b0/0x24c
>   driver_register+0x9c/0xe0
>   pcie_port_service_register+0x64/0x7c
>   pcied_init+0x44/0xa4
>   do_one_initcall+0x1d0/0x1f0
>   kernel_init_freeable+0x24c/0x254
>   kernel_init+0x18/0xe8
>   ret_from_fork+0x10/0x20
> 
> Fixes: 8a94644b440e ("PCI: Fix pci_create_slot() reference count leak")
> Signed-off-by: Jubin Zhong <zhongjubin@huawei.com>
> Cc: stable@vger.kernel.org #v4.4.235

I'm curious how you noticed this.  Did kobject_init_and_add() fail for
some reason?  Is there a bug there that we need to fix also?

And why did you mention v4.4.235 in the stable tag?  8a94644b440e
wasn't merged until v5.9.  If 8a94644b440e was backported to older
kernels, we can't know that in general, so I think whoever backported
it is responsible for watching for Fixes tags that mention it.

I updated the stable tag to "v5.9+" and applied this to pci/hotplug
for v5.11, thanks!

> ----
> v2:
>   Since both slot memory and slot->list would be handled by
> pci_slot_release(), we need to make sure slot->list is properly
> initialized beforehand.
> 
> v1: https://lore.kernel.org/lkml/1606288971-47927-1-git-send-email-zhongjubin@huawei.com/
>   Two things need to be cleaned up on pci_create_slot's error path:
>   1. free slot memory
>   2. remove slot->list from its parent->slots
>   This patch mistakenly took slot memory as unfreed (which is not), and
> would introduce double free problem.
> ---
>  drivers/pci/slot.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 3861505..ed2077e 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -272,6 +272,9 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  		goto err;
>  	}
>  
> +	INIT_LIST_HEAD(&slot->list);
> +	list_add(&slot->list, &parent->slots);
> +
>  	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
>  				   "%s", slot_name);
>  	if (err) {
> @@ -279,9 +282,6 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  		goto err;
>  	}
>  
> -	INIT_LIST_HEAD(&slot->list);
> -	list_add(&slot->list, &parent->slots);
> -
>  	down_read(&pci_bus_sem);
>  	list_for_each_entry(dev, &parent->devices, bus_list)
>  		if (PCI_SLOT(dev->devfn) == slot_nr)
> -- 
> 1.8.5.6
> 

Re: Re: [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()

[zhongjubin]

>On Wed, Dec 02, 2020 at 10:33:42AM +0800, Jubin Zhong wrote:
>> Once kobject_init_and_add() failed, pci_slot_release() is called to 
>> delete slot->list from parent->slots. But slot->list is intialized 
>> afterwards, so we ran into the following crash:
>> 
>>   Unable to handle kernel NULL pointer dereference at virtual address
>> 00000000
>>   ...
>>   CPU: 10 PID: 1 Comm: swapper/0 Not tainted 4.4.240 #197
>>   task: ffffeb398a45ef10 task.stack: ffffeb398a470000
>>   PC is at __list_del_entry_valid+0x5c/0xb0
>>   LR is at pci_slot_release+0x84/0xe4
>>   ...
>>   __list_del_entry_valid+0x5c/0xb0
>>   pci_slot_release+0x84/0xe4
>>   kobject_put+0x184/0x1c4
>>   pci_create_slot+0x17c/0x1b4
>>   __pci_hp_initialize+0x68/0xa4
>>   pciehp_probe+0x1a4/0x2fc
>>   pcie_port_probe_service+0x58/0x84
>>   driver_probe_device+0x320/0x470
>>   __driver_attach+0x54/0xb8
>>   bus_for_each_dev+0xc8/0xf0
>>   driver_attach+0x30/0x3c
>>   bus_add_driver+0x1b0/0x24c
>>   driver_register+0x9c/0xe0
>>   pcie_port_service_register+0x64/0x7c
>>   pcied_init+0x44/0xa4
>>   do_one_initcall+0x1d0/0x1f0
>>   kernel_init_freeable+0x24c/0x254
>>   kernel_init+0x18/0xe8
>>   ret_from_fork+0x10/0x20
>> 
>> Fixes: 8a94644b440e ("PCI: Fix pci_create_slot() reference count 
>> leak")
>> Signed-off-by: Jubin Zhong <zhongjubin@huawei.com>
>> Cc: stable@vger.kernel.org #v4.4.235

>I'm curious how you noticed this.  Did kobject_init_and_add() fail for some reason?  Is there a bug there that we need to fix also?

Actually not.
In the very beginning, Coverity reported a RESOURCE_LEAK warning of slot memory. It should be a false alarm because pci_slot_release() is already called to free slot memory as a function hook (which Coverity could not recognize). After code review I prepared to cancel the warning, that's when I noticed the unintialized slot list is also deleted by pci_slot_release().

>And why did you mention v4.4.235 in the stable tag?  8a94644b440e wasn't merged until v5.9.  If 8a94644b440e was backported to older kernels, we can't know that in general, so I think whoever backported it is responsible for watching for Fixes tags that mention it.

The Coverity warning was reported on v4.4.240. I also did a little test on v4.4.240 by forcing kobject_init_and_add() to fail, that's how I got the above call stack. Since 8a94644b440e was backported to v4.4.235, I thought it should be necessary to fix it on v4.4 too.

>>I updated the stable tag to "v5.9+" and applied this to pci/hotplug for v5.11, thanks!
 
>> ----
>> v2:
>>   Since both slot memory and slot->list would be handled by 
>> pci_slot_release(), we need to make sure slot->list is properly 
>> initialized beforehand.
>> 
>> v1: https://lore.kernel.org/lkml/1606288971-47927-1-git-send-email-zhongjubin@huawei.com/
>>   Two things need to be cleaned up on pci_create_slot's error path:
>>   1. free slot memory
>>   2. remove slot->list from its parent->slots
>>   This patch mistakenly took slot memory as unfreed (which is not), 
>> and would introduce double free problem.
>> ---
>>  drivers/pci/slot.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c index 
>> 3861505..ed2077e 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -272,6 +272,9 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>>  		goto err;
>>  	}
>>  
>> +	INIT_LIST_HEAD(&slot->list);
>> +	list_add(&slot->list, &parent->slots);
>> +
>>  	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
>>  				   "%s", slot_name);
>>  	if (err) {
>> @@ -279,9 +282,6 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>>  		goto err;
>>  	}
>>  
>> -	INIT_LIST_HEAD(&slot->list);
>> -	list_add(&slot->list, &parent->slots);
>> -
>>  	down_read(&pci_bus_sem);
>>  	list_for_each_entry(dev, &parent->devices, bus_list)
>>  		if (PCI_SLOT(dev->devfn) == slot_nr)
>> --
>> 1.8.5.6
>>