* [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()
@ 2020-12-02 2:33 Jubin Zhong
2020-12-04 23:37 ` Bjorn Helgaas
0 siblings, 1 reply; 3+ messages in thread
From: Jubin Zhong @ 2020-12-02 2:33 UTC (permalink / raw)
To: bhelgaas
Cc: liuwenliang, wangfangpeng1, nixiaoming, gregkh, wu000273,
linux-pci, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()
2020-12-02 2:33 [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release() Jubin Zhong
@ 2020-12-04 23:37 ` Bjorn Helgaas
2020-12-07 9:52 ` zhongjubin
0 siblings, 1 reply; 3+ messages in thread
From: Bjorn Helgaas @ 2020-12-04 23:37 UTC (permalink / raw)
To: Jubin Zhong
Cc: bhelgaas, liuwenliang, wangfangpeng1, nixiaoming, gregkh,
wu000273, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: Re: [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release()
2020-12-04 23:37 ` Bjorn Helgaas
@ 2020-12-07 9:52 ` zhongjubin
0 siblings, 0 replies; 3+ messages in thread
From: zhongjubin @ 2020-12-07 9:52 UTC (permalink / raw)
To: helgaas
Cc: bhelgaas, liuwenliang, wangfangpeng1, nixiaoming, gregkh,
wu000273, linux-pci, linux-kernel
>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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-07 9:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 2:33 [PATCH v2] PCI: Fix Oops caused by uninitialized slot->list in pci_slot_release() Jubin Zhong
2020-12-04 23:37 ` Bjorn Helgaas
2020-12-07 9:52 ` zhongjubin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).