linux-kernel - Re: [PATCH 6.13] PCI/pwrctrl: Skip NULL of

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241129192811.GA2768738@bhelgaas>
Date: Fri, 29 Nov 2024 13:28:11 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Brian Norris <briannorris@...omium.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	Jon Hunter <jonathanh@...dia.com>,
	Saurabh Sengar <ssengar@...ux.microsoft.com>
Subject: Re: [PATCH 6.13] PCI/pwrctrl: Skip NULL of_node when unregistering

[+cc Jon, Saurabh]

On Tue, Nov 26, 2024 at 01:04:34PM -0800, Brian Norris wrote:
> of_find_device_by_node() doesn't like a NULL pointer, and may end up
> identifying an arbitrary device, which we then start tearing down. We
> should check for NULL first.

I assume pci_pwrctrl_unregister() is unregistering something added by
pci_pwrctrl_create_devices() in pci_bus_add_device()?

There are a couple things I don't like about this code (these are
unrelated to this particular regression fix, but they make me worry
that I'm misunderstanding how this all works):

  - pci_pwrctrl_create_devices() and pci_pwrctrl_unregister() are not
    parallel names.

  - pci_pwrctrl_create_devices() has a loop and potentially creates
    several pwrctrl devices, one for each child that has a "*-supply"
    property, but there is no corresponding loop in
    pci_pwrctrl_unregister().

I see that 681725afb6b9 ("PCI/pwrctl: Remove pwrctl device without
iterating over all children of pwrctl parent") is at least partly
*about* removing without iterating, and the commit log has some
explanation about how pwrctrl devices lying around will get removed,
but the lack of parallelism is a hassle for readers.

The subject and commit log also refer to "*the* pwrctl device," which
suggests there is only one, but that seems slightly misleading even
though it goes on to mention "any pwrctl devices lying around."

If they get removed when the parent gets removed, why do we need to
remove *any* of them?  Is the real point that we need to clear
OF_POPULATED?

Maybe I'm missing the whole point here?

> [  222.965216] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000c0
> ...
> [  223.107395] CPU: 3 UID: 0 PID: 5095 Comm: bash Tainted: G        WC         6.12.0-rc1 #3
> ...
> [  223.227750] Call trace:
> [  223.230501]  pci_stop_bus_device+0x190/0x1b0
> [  223.235314]  pci_stop_and_remove_bus_device_locked+0x28/0x48
> [  223.241672]  remove_store+0xa0/0xb8
> [  223.245616]  dev_attr_store+0x20/0x40
> [  223.249737]  sysfs_kf_write+0x4c/0x68
> [  223.253859]  kernfs_fop_write_iter+0x128/0x200
> [  223.253887]  do_iter_readv_writev+0xdc/0x1e0
> [  223.263631]  vfs_writev+0x100/0x2a0
> [  223.267550]  do_writev+0x84/0x130
> [  223.271273]  __arm64_sys_writev+0x28/0x40
> [  223.275774]  invoke_syscall+0x50/0x120
> [  223.279988]  el0_svc_common.constprop.0+0x48/0xf0
> [  223.285270]  do_el0_svc+0x24/0x38
> [  223.288993]  el0_svc+0x34/0xe0
> [  223.292426]  el0t_64_sync_handler+0x120/0x138
> [  223.297311]  el0t_64_sync+0x190/0x198
> [  223.301423] Code: 17fffff8 91030000 d2800101 f9800011 (c85f7c02)
> [  223.308248] ---[ end trace 0000000000000000 ]---
> 
> Fixes: 681725afb6b9 ("PCI/pwrctl: Remove pwrctl device without iterating over all children of pwrctl parent")
> Signed-off-by: Brian Norris <briannorris@...omium.org>
> ---
> 
>  drivers/pci/remove.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 963b8d2855c1..efc37fcb73e2 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -19,14 +19,19 @@ static void pci_free_resources(struct pci_dev *dev)
>  
>  static void pci_pwrctrl_unregister(struct device *dev)
>  {
> +	struct device_node *np;
>  	struct platform_device *pdev;
>  
> -	pdev = of_find_device_by_node(dev_of_node(dev));
> +	np = dev_of_node(dev);
> +	if (!np)
> +		return;
> +
> +	pdev = of_find_device_by_node(np);
>  	if (!pdev)
>  		return;
>  
>  	of_device_unregister(pdev);
> -	of_node_clear_flag(dev_of_node(dev), OF_POPULATED);
> +	of_node_clear_flag(np, OF_POPULATED);
>  }
>  
>  static void pci_stop_dev(struct pci_dev *dev)
> -- 
> 2.47.0.338
>