[PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter

[PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter

[Logan Gunthorpe]

The disable_acs_redir parameter stores a pointer to the string passed to
pci_setup(). However, the string passed to PCI setup is actually a
temporary copy allocated in static __initdata memory. After init, once
the memory is freed, it is no longer valid to reference this pointer.

This bug was noticed in v5.0-rc1 after a change in commit c5eb1190074c
("PCI / PM: Allow runtime PM without callback functions") caused
pci_disable_acs_redir() to be called during shutdown which manifested
as an unable to handle kernel paging request at:

RIP: 0010:pci_enable_acs+0x3f/0x1e0
Call Trace:
   pci_restore_state.part.44+0x159/0x3c0
   pci_restore_standard_config+0x33/0x40
   pci_pm_runtime_resume+0x2b/0xd0
   ? pci_restore_standard_config+0x40/0x40
   __rpm_callback+0xbc/0x1b0
   rpm_callback+0x1f/0x70
   ? pci_restore_standard_config+0x40/0x40
    rpm_resume+0x4f9/0x710
   ? pci_conf1_read+0xb6/0xf0
   ? pci_conf1_write+0xb2/0xe0
   __pm_runtime_resume+0x47/0x70
   pci_device_shutdown+0x1e/0x60
   device_shutdown+0x14a/0x1f0
   kernel_restart+0xe/0x50
   __do_sys_reboot+0x1ee/0x210
   ? __fput+0x144/0x1d0
   do_writev+0x5e/0xf0
   ? do_writev+0x5e/0xf0
   do_syscall_64+0x48/0xf0
   entry_SYSCALL_64_after_hwframe+0x44/0xa9

It was also likely possible to trigger this bug when hotplugging PCI
devices.

To fix this, instead of storing a pointer, we use kstrdup to copy the
disable_acs_redir_param to its own buffer which will never be freed.

Fixes: aaca43fda742 ("PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9d8e3c837de..c25acace7d91 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6195,7 +6195,8 @@ static int __init pci_setup(char *str)
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
 			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
-				disable_acs_redir_param = str + 18;
+				disable_acs_redir_param =
+					kstrdup(str + 18, GFP_KERNEL);
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);
-- 
2.19.0

Re: [PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter

[Jarkko Nikula]

Hi

On 1/15/19 7:32 PM, Logan Gunthorpe wrote:
> The disable_acs_redir parameter stores a pointer to the string passed to
> pci_setup(). However, the string passed to PCI setup is actually a
> temporary copy allocated in static __initdata memory. After init, once
> the memory is freed, it is no longer valid to reference this pointer.
> 
> This bug was noticed in v5.0-rc1 after a change in commit c5eb1190074c
> ("PCI / PM: Allow runtime PM without callback functions") caused
> pci_disable_acs_redir() to be called during shutdown which manifested
> as an unable to handle kernel paging request at:
> 
> RIP: 0010:pci_enable_acs+0x3f/0x1e0
> Call Trace:
>     pci_restore_state.part.44+0x159/0x3c0
>     pci_restore_standard_config+0x33/0x40
>     pci_pm_runtime_resume+0x2b/0xd0
>     ? pci_restore_standard_config+0x40/0x40
>     __rpm_callback+0xbc/0x1b0
>     rpm_callback+0x1f/0x70
>     ? pci_restore_standard_config+0x40/0x40
>      rpm_resume+0x4f9/0x710
>     ? pci_conf1_read+0xb6/0xf0
>     ? pci_conf1_write+0xb2/0xe0
>     __pm_runtime_resume+0x47/0x70
>     pci_device_shutdown+0x1e/0x60

So this doesn't happen if you revert c5eb1190074c?

I guess this is due dev->state_saved being true set by 
pci_pm_runtime_suspend() -> pci_save_state() after my patch and now 
pci_pm_runtime_resume() -> pci_restore_standard_config() -> 
pci_restore_state() reach the pci_enable_acs(). I think this is possible 
to trigger also before my patch if device has the runtime PM callback 
defined?

> It was also likely possible to trigger this bug when hotplugging PCI
> devices.
> 
> To fix this, instead of storing a pointer, we use kstrdup to copy the
> disable_acs_redir_param to its own buffer which will never be freed.
> 
I wasn't able to trigger this but I saw
"PCI: Can't parse disable_acs_redir parameter: " followed by a few lines 
of junk during boot when I defined pci=disable_acs_redir=0000:00:xy.z 
which disappear after your patch.

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter

[Logan Gunthorpe]

On 2019-01-16 2:37 a.m., Jarkko Nikula wrote:
> So this doesn't happen if you revert c5eb1190074c?

Yes, but the bug was fully mine in the disable_acs_redir code. That
patch only just made it noticable to me.

> I guess this is due dev->state_saved being true set by 
> pci_pm_runtime_suspend() -> pci_save_state() after my patch and now 
> pci_pm_runtime_resume() -> pci_restore_standard_config() -> 
> pci_restore_state() reach the pci_enable_acs(). I think this is possible 
> to trigger also before my patch if device has the runtime PM callback 
> defined?

Yes, I also think it was possible to trigger without that patch. I just
never hit it.

> I wasn't able to trigger this but I saw
> "PCI: Can't parse disable_acs_redir parameter: " followed by a few lines 
> of junk during boot when I defined pci=disable_acs_redir=0000:00:xy.z 
> which disappear after your patch.

Yes, I guess in theory, it will also depend what happens to the RAM the
parameter pointer points too. On my system it's panicing trying to
access unallocated RAM, on yours it may be reading RAM that's used for
another purpose and spewing garbage because of it.

> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Thanks!

Logan

Re: [PATCH] PCI: fix using __initdata memory after free in disable_acs_redir parameter

[Bjorn Helgaas]

On Tue, Jan 15, 2019 at 10:32:03AM -0700, Logan Gunthorpe wrote:
> The disable_acs_redir parameter stores a pointer to the string passed to
> pci_setup(). However, the string passed to PCI setup is actually a
> temporary copy allocated in static __initdata memory. After init, once
> the memory is freed, it is no longer valid to reference this pointer.
> 
> This bug was noticed in v5.0-rc1 after a change in commit c5eb1190074c
> ("PCI / PM: Allow runtime PM without callback functions") caused
> pci_disable_acs_redir() to be called during shutdown which manifested
> as an unable to handle kernel paging request at:
> 
> RIP: 0010:pci_enable_acs+0x3f/0x1e0
> Call Trace:
>    pci_restore_state.part.44+0x159/0x3c0
>    pci_restore_standard_config+0x33/0x40
>    pci_pm_runtime_resume+0x2b/0xd0
>    ? pci_restore_standard_config+0x40/0x40
>    __rpm_callback+0xbc/0x1b0
>    rpm_callback+0x1f/0x70
>    ? pci_restore_standard_config+0x40/0x40
>     rpm_resume+0x4f9/0x710
>    ? pci_conf1_read+0xb6/0xf0
>    ? pci_conf1_write+0xb2/0xe0
>    __pm_runtime_resume+0x47/0x70
>    pci_device_shutdown+0x1e/0x60
>    device_shutdown+0x14a/0x1f0
>    kernel_restart+0xe/0x50
>    __do_sys_reboot+0x1ee/0x210
>    ? __fput+0x144/0x1d0
>    do_writev+0x5e/0xf0
>    ? do_writev+0x5e/0xf0
>    do_syscall_64+0x48/0xf0
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> It was also likely possible to trigger this bug when hotplugging PCI
> devices.
> 
> To fix this, instead of storing a pointer, we use kstrdup to copy the
> disable_acs_redir_param to its own buffer which will never be freed.
> 
> Fixes: aaca43fda742 ("PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>

Applied with Jarkko's tested- and reviewed-by to for-linus for v5.0,
thanks!

> ---
>  drivers/pci/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index c9d8e3c837de..c25acace7d91 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6195,7 +6195,8 @@ static int __init pci_setup(char *str)
>  			} else if (!strncmp(str, "pcie_scan_all", 13)) {
>  				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>  			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
> -				disable_acs_redir_param = str + 18;
> +				disable_acs_redir_param =
> +					kstrdup(str + 18, GFP_KERNEL);
>  			} else {
>  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
>  						str);
> -- 
> 2.19.0
>