[PATCH V3, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[PATCH V3, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

Hi Bjorn,
I reworked the patch as suggested, would you please kindly
review the patch? By the way, the use-after-free issue would not
impact DPC, according to PCI Express Base Specification
Revision 4.0 Version 0.3 The DPC "capability may be
implemented by a Root Port or a Switch Downstream Port.
It is not applicable to any other Device/Port type."
The use-after-free only happens on a non-bridge device, so
it won't impact root/switch port.

Thank you very much.
Thomas

[PATCH V3, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

When an fatal error is recevied by a non-bridge device,
the device is removed from the pci bus and the device structure
is freed by pci_stop_and_remove_bus_device(). The freed device
structure is used in the subsequence pci_info() to printout the
message. It causes a corrupt printout. If slub_debug=FZP is used,
it will cause following protection fault after a fatal error is
received.

general protection fault: 0000 [#1] SMP PTI
CPU: 104 PID: 1077 Comm: kworker/104:1 Not tainted 4.18.0-rc1ttai #5
Hardware name: Oracle Corporation ORACLE SERVER X5-4/ASSY,MB WITH TRAY,
BIOS 36030500 11/16/2016
Workqueue: events aer_isr
 RIP: 0010:__dev_printk+0x2e/0x90
 Code: 00 55 49 89 d1 48 89 e5 53 48 89 fb 48 83 ec 18 48 85 f6
 74 5f 4c 8b 46 50 4d 85 c0 74 2b 48 8b 86 88 00 00 00 48 85 c0
 74 25 <48> 8b 08 0f be 7b 01 48 c7 c2 83 d4 71 99 31 c0 83 ef
 30 e8 4a ff
 RSP: 0018:ffffb6b88fa57cf8 EFLAGS: 00010202
 RAX: 6b6b6b6b6b6b6b6b RBX: ffffffff996ba720 RCX: 0000000000000000
 RDX: ffffb6b88fa57d28 RSI: ffff8c4d7af94128 RDI: ffffffff996ba720
 RBP: ffffb6b88fa57d18 R08: 6b6b6b6b6b6b6b6b R09: ffffb6b88fa57d28
 R10: ffffffff99baca80 R11: 0000000000000000 R12: ffff8c4d7ae95990
 R13: ffff8c2d7a840008 R14: ffff8c4d7af94088 R15: ffff8c4d7af90008
 FS:  0000000000000000(0000) GS:ffff8c2d7fc00000(0000)
knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f22c0839000 CR3: 000000136bc0a001 CR4: 00000000001606e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  ? pci_bus_add_device+0x4f/0xa0
  _dev_info+0x6c/0x90
  pcie_do_fatal_recovery+0x1d5/0x230
  aer_isr+0x3e5/0x950
  ? add_timer_on+0xcc/0x160
  process_one_work+0x168/0x370
  worker_thread+0x4f/0x3d0
  kthread+0x105/0x140
  ? max_active_store+0x80/0x80
  ? kthread_bind+0x20/0x20
  ret_from_fork+0x35/0x40

To fix this issue, pci_dev_get is used to keep the device around.
After all error devices are processed, pci_dev_put is then called
to decrement the reference count for all error devices.

Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
---
 drivers/pci/pcie/aer.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index a2e8838..6e5e6a5 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -657,6 +657,10 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
 static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 {
 	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
+		/* increment reference count to keep the dev
+		 * around until remove_source_device()
+		 */
+		pci_dev_get(dev);
 		e_info->dev[e_info->error_dev_num] = dev;
 		e_info->error_dev_num++;
 		return 0;
@@ -665,6 +669,21 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 }
 
 /**
+ * remove_source_device -remove error devices from the e_info
+ * @e_info: pointer to error info
+ */
+static void remove_source_device(struct aer_err_info *e_info)
+{
+	struct pci_dev *dev;
+
+	while (e_info->error_dev_num > 0) {
+		e_info->error_dev_num--;
+		dev = e_info->dev[e_info->error_dev_num];
+		pci_dev_put(dev);
+	}
+}
+
+/**
  * is_error_source - check whether the device is source of reported error
  * @dev: pointer to pci_dev to be checked
  * @e_info: pointer to reported error info
@@ -976,8 +995,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 			e_info->multi_error_valid = 0;
 		aer_print_port_info(pdev, e_info);
 
-		if (find_source_device(pdev, e_info))
+		if (find_source_device(pdev, e_info)) {
 			aer_process_err_devices(e_info);
+			remove_source_device(e_info);
+		}
 	}
 
 	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
@@ -995,8 +1016,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 
 		aer_print_port_info(pdev, e_info);
 
-		if (find_source_device(pdev, e_info))
+		if (find_source_device(pdev, e_info)) {
 			aer_process_err_devices(e_info);
+			remove_source_device(e_info);
+		}
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH V3, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Bjorn Helgaas]

On Thu, Jul 19, 2018 at 02:02:35PM -0600, Thomas Tai wrote:
> When an fatal error is recevied by a non-bridge device,
> the device is removed from the pci bus and the device structure
> is freed by pci_stop_and_remove_bus_device(). The freed device
> structure is used in the subsequence pci_info() to printout the
> message. It causes a corrupt printout. If slub_debug=FZP is used,
> it will cause following protection fault after a fatal error is
> received.
> 
> general protection fault: 0000 [#1] SMP PTI
> CPU: 104 PID: 1077 Comm: kworker/104:1 Not tainted 4.18.0-rc1ttai #5
> Hardware name: Oracle Corporation ORACLE SERVER X5-4/ASSY,MB WITH TRAY,
> BIOS 36030500 11/16/2016
> Workqueue: events aer_isr
>  RIP: 0010:__dev_printk+0x2e/0x90
>  Code: 00 55 49 89 d1 48 89 e5 53 48 89 fb 48 83 ec 18 48 85 f6
>  74 5f 4c 8b 46 50 4d 85 c0 74 2b 48 8b 86 88 00 00 00 48 85 c0
>  74 25 <48> 8b 08 0f be 7b 01 48 c7 c2 83 d4 71 99 31 c0 83 ef
>  30 e8 4a ff
>  RSP: 0018:ffffb6b88fa57cf8 EFLAGS: 00010202
>  RAX: 6b6b6b6b6b6b6b6b RBX: ffffffff996ba720 RCX: 0000000000000000
>  RDX: ffffb6b88fa57d28 RSI: ffff8c4d7af94128 RDI: ffffffff996ba720
>  RBP: ffffb6b88fa57d18 R08: 6b6b6b6b6b6b6b6b R09: ffffb6b88fa57d28
>  R10: ffffffff99baca80 R11: 0000000000000000 R12: ffff8c4d7ae95990
>  R13: ffff8c2d7a840008 R14: ffff8c4d7af94088 R15: ffff8c4d7af90008
>  FS:  0000000000000000(0000) GS:ffff8c2d7fc00000(0000)
> knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f22c0839000 CR3: 000000136bc0a001 CR4: 00000000001606e0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   ? pci_bus_add_device+0x4f/0xa0
>   _dev_info+0x6c/0x90
>   pcie_do_fatal_recovery+0x1d5/0x230
>   aer_isr+0x3e5/0x950
>   ? add_timer_on+0xcc/0x160
>   process_one_work+0x168/0x370
>   worker_thread+0x4f/0x3d0
>   kthread+0x105/0x140
>   ? max_active_store+0x80/0x80
>   ? kthread_bind+0x20/0x20
>   ret_from_fork+0x35/0x40
> 
> To fix this issue, pci_dev_get is used to keep the device around.
> After all error devices are processed, pci_dev_put is then called
> to decrement the reference count for all error devices.

I know I suggested this strategy, but I think this ended up being more
complicated than it's worth.

The problem code in pcie_do_fatal_recovery() essentially looks like
this:

  pcie_do_fatal_recovery(dev)
    pci_stop_and_remove_bus_device(dev);
    reset_link(dev);
    pci_cleanup_aer_uncorrect_error_status(dev);
    pcie_wait_for_link(dev, ...);
    pci_uevent_ers(dev, ...);
    pci_info(dev, ...);

Some of this depends on the device type (bridge vs. endpoint) and the
caller (AER vs. DPC), but given the right conditions, we can exercise
all the above calls.

I think it is just broken that we keep doing things with "dev" after
removing it.  IMHO this code should be restructured to avoid that.

I think fiddling with the refcount as in this patch adds too much
complexity and makes it look like the current structure of
pcie_do_fatal_recovery() is reasonable when it really isn't.

But restructuring pcie_do_fatal_recovery() is too big a project to do
before v4.18, and we need to fix this problem.  I propose that we
merge your v2 patch for now, so at least the band-aid is in the
function that I think is broken.

I *would* like to reduce the scope of the get/put as in the patch
below, though, so it is contained inside the rescan_remove lock.
Could you try it and make sure it's still enough to avoid the problem?
If it is, I'll add your sign-off and get this in v4.18.


commit 277ce38f2ed6a4310acf3bd541fb3aee4ec27dee
Author: Thomas Tai <thomas.tai@oracle.com>
Date:   Tue Jul 24 16:47:59 2018 -0500

    PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()
    
    When an fatal error is received by a non-bridge device, the device is
    removed, and pci_stop_and_remove_bus_device() deallocates the device
    structure.  The freed device structure is used by subsequent code to send
    uevents and print messages.
    
    Hold a reference on the device until we're finished using it.  This is not
    an ideal fix because pcie_do_fatal_recovery() should not use the device at
    all after removing it, but that's too big a project for right now.
    
    #
    [bhelgaas: changelog, reduce get/put coverage]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
index fdbcc555860d..674984a9277a 100644
--- a/drivers/pci/pcie/err.c
+++ b/drivers/pci/pcie/err.c
@@ -291,6 +291,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 
 	parent = udev->subordinate;
 	pci_lock_rescan_remove();
+	pci_dev_get(dev);
 	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
 					 bus_list) {
 		pci_dev_get(pdev);
@@ -325,6 +326,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
 		pci_info(dev, "Device recovery from fatal error failed\n");
 	}
 
+	pci_dev_put(dev);
 	pci_unlock_rescan_remove();
 }
 

> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
> ---
>  drivers/pci/pcie/aer.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e8838..6e5e6a5 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -657,6 +657,10 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
>  static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  {
>  	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
> +		/* increment reference count to keep the dev
> +		 * around until remove_source_device()
> +		 */
> +		pci_dev_get(dev);
>  		e_info->dev[e_info->error_dev_num] = dev;
>  		e_info->error_dev_num++;
>  		return 0;
> @@ -665,6 +669,21 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  }
>  
>  /**
> + * remove_source_device -remove error devices from the e_info
> + * @e_info: pointer to error info
> + */
> +static void remove_source_device(struct aer_err_info *e_info)
> +{
> +	struct pci_dev *dev;
> +
> +	while (e_info->error_dev_num > 0) {
> +		e_info->error_dev_num--;
> +		dev = e_info->dev[e_info->error_dev_num];
> +		pci_dev_put(dev);
> +	}
> +}
> +
> +/**
>   * is_error_source - check whether the device is source of reported error
>   * @dev: pointer to pci_dev to be checked
>   * @e_info: pointer to reported error info
> @@ -976,8 +995,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  			e_info->multi_error_valid = 0;
>  		aer_print_port_info(pdev, e_info);
>  
> -		if (find_source_device(pdev, e_info))
> +		if (find_source_device(pdev, e_info)) {
>  			aer_process_err_devices(e_info);
> +			remove_source_device(e_info);
> +		}
>  	}
>  
>  	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
> @@ -995,8 +1016,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  
>  		aer_print_port_info(pdev, e_info);
>  
> -		if (find_source_device(pdev, e_info))
> +		if (find_source_device(pdev, e_info)) {
>  			aer_process_err_devices(e_info);
> +			remove_source_device(e_info);
> +		}
>  	}
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH V3, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

[ ... ]>
> I know I suggested this strategy, but I think this ended up being more
> complicated than it's worth.
> 
> The problem code in pcie_do_fatal_recovery() essentially looks like
> this:
> 
>    pcie_do_fatal_recovery(dev)
>      pci_stop_and_remove_bus_device(dev);
>      reset_link(dev);
>      pci_cleanup_aer_uncorrect_error_status(dev);
>      pcie_wait_for_link(dev, ...);
>      pci_uevent_ers(dev, ...);
>      pci_info(dev, ...);
> 
> Some of this depends on the device type (bridge vs. endpoint) and the
> caller (AER vs. DPC), but given the right conditions, we can exercise
> all the above calls.
> 
> I think it is just broken that we keep doing things with "dev" after
> removing it.  IMHO this code should be restructured to avoid that.
> 
> I think fiddling with the refcount as in this patch adds too much
> complexity and makes it look like the current structure of
> pcie_do_fatal_recovery() is reasonable when it really isn't.
> 
> But restructuring pcie_do_fatal_recovery() is too big a project to do
> before v4.18, and we need to fix this problem.  I propose that we
> merge your v2 patch for now, so at least the band-aid is in the
> function that I think is broken.
> 
> I *would* like to reduce the scope of the get/put as in the patch
> below, though, so it is contained inside the rescan_remove lock.
> Could you try it and make sure it's still enough to avoid the problem?
> If it is, I'll add your sign-off and get this in v4.18.

Hi Bjorn,
Thank you for your review and the details analysis. Sure, let's do the 
work around for now. I retested your patch below and works fine. You are 
welcome to add my signed-off and get this in v4.18.

As far as reworking the pcie_do_fatal_recovery() goes, would you think I 
can help out in any way? May be I can try rework the code to not use the 
dev after it is removed.

Thank you,
Thomas

> 
> 
> commit 277ce38f2ed6a4310acf3bd541fb3aee4ec27dee
> Author: Thomas Tai <thomas.tai@oracle.com>
> Date:   Tue Jul 24 16:47:59 2018 -0500
> 
>      PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()
>      
>      When an fatal error is received by a non-bridge device, the device is
>      removed, and pci_stop_and_remove_bus_device() deallocates the device
>      structure.  The freed device structure is used by subsequent code to send
>      uevents and print messages.
>      
>      Hold a reference on the device until we're finished using it.  This is not
>      an ideal fix because pcie_do_fatal_recovery() should not use the device at
>      all after removing it, but that's too big a project for right now.
>      
>      #
>      [bhelgaas: changelog, reduce get/put coverage]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index fdbcc555860d..674984a9277a 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -291,6 +291,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>   
>   	parent = udev->subordinate;
>   	pci_lock_rescan_remove();
> +	pci_dev_get(dev);
>   	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>   					 bus_list) {
>   		pci_dev_get(pdev);
> @@ -325,6 +326,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>   		pci_info(dev, "Device recovery from fatal error failed\n");
>   	}
>   
> +	pci_dev_put(dev);
>   	pci_unlock_rescan_remove();
>   }
>   
> 
>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>> ---
>>   drivers/pci/pcie/aer.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index a2e8838..6e5e6a5 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -657,6 +657,10 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
>>   static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>>   {
>>   	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
>> +		/* increment reference count to keep the dev
>> +		 * around until remove_source_device()
>> +		 */
>> +		pci_dev_get(dev);
>>   		e_info->dev[e_info->error_dev_num] = dev;
>>   		e_info->error_dev_num++;
>>   		return 0;
>> @@ -665,6 +669,21 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>>   }
>>   
>>   /**
>> + * remove_source_device -remove error devices from the e_info
>> + * @e_info: pointer to error info
>> + */
>> +static void remove_source_device(struct aer_err_info *e_info)
>> +{
>> +	struct pci_dev *dev;
>> +
>> +	while (e_info->error_dev_num > 0) {
>> +		e_info->error_dev_num--;
>> +		dev = e_info->dev[e_info->error_dev_num];
>> +		pci_dev_put(dev);
>> +	}
>> +}
>> +
>> +/**
>>    * is_error_source - check whether the device is source of reported error
>>    * @dev: pointer to pci_dev to be checked
>>    * @e_info: pointer to reported error info
>> @@ -976,8 +995,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>>   			e_info->multi_error_valid = 0;
>>   		aer_print_port_info(pdev, e_info);
>>   
>> -		if (find_source_device(pdev, e_info))
>> +		if (find_source_device(pdev, e_info)) {
>>   			aer_process_err_devices(e_info);
>> +			remove_source_device(e_info);
>> +		}
>>   	}
>>   
>>   	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
>> @@ -995,8 +1016,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>>   
>>   		aer_print_port_info(pdev, e_info);
>>   
>> -		if (find_source_device(pdev, e_info))
>> +		if (find_source_device(pdev, e_info)) {
>>   			aer_process_err_devices(e_info);
>> +			remove_source_device(e_info);
>> +		}
>>   	}
>>   }
>>   
>> -- 
>> 1.8.3.1
>>

Re: [PATCH V3, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Bjorn Helgaas]

On Thu, Jul 26, 2018 at 10:29:18AM -0400, Thomas Tai wrote:
> [ ... ]>
> > I know I suggested this strategy, but I think this ended up being more
> > complicated than it's worth.
> > 
> > The problem code in pcie_do_fatal_recovery() essentially looks like
> > this:
> > 
> >    pcie_do_fatal_recovery(dev)
> >      pci_stop_and_remove_bus_device(dev);
> >      reset_link(dev);
> >      pci_cleanup_aer_uncorrect_error_status(dev);
> >      pcie_wait_for_link(dev, ...);
> >      pci_uevent_ers(dev, ...);
> >      pci_info(dev, ...);
> > 
> > Some of this depends on the device type (bridge vs. endpoint) and the
> > caller (AER vs. DPC), but given the right conditions, we can exercise
> > all the above calls.
> > 
> > I think it is just broken that we keep doing things with "dev" after
> > removing it.  IMHO this code should be restructured to avoid that.
> > 
> > I think fiddling with the refcount as in this patch adds too much
> > complexity and makes it look like the current structure of
> > pcie_do_fatal_recovery() is reasonable when it really isn't.
> > 
> > But restructuring pcie_do_fatal_recovery() is too big a project to do
> > before v4.18, and we need to fix this problem.  I propose that we
> > merge your v2 patch for now, so at least the band-aid is in the
> > function that I think is broken.
> > 
> > I *would* like to reduce the scope of the get/put as in the patch
> > below, though, so it is contained inside the rescan_remove lock.
> > Could you try it and make sure it's still enough to avoid the problem?
> > If it is, I'll add your sign-off and get this in v4.18.
> 
> Hi Bjorn,
> Thank you for your review and the details analysis. Sure, let's do the work
> around for now. I retested your patch below and works fine. You are welcome
> to add my signed-off and get this in v4.18.

OK, I added your signed-off-by and put the patch below on my for-linus
branch for v4.18.

> As far as reworking the pcie_do_fatal_recovery() goes, would you think I can
> help out in any way? May be I can try rework the code to not use the dev
> after it is removed.

That'd be great!  I expect Oza and Keith will have useful insight
there, too, so keep them in the loop.

> > commit 277ce38f2ed6a4310acf3bd541fb3aee4ec27dee
> > Author: Thomas Tai <thomas.tai@oracle.com>
> > Date:   Tue Jul 24 16:47:59 2018 -0500
> > 
> >      PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()
> >      When an fatal error is received by a non-bridge device, the device is
> >      removed, and pci_stop_and_remove_bus_device() deallocates the device
> >      structure.  The freed device structure is used by subsequent code to send
> >      uevents and print messages.
> >      Hold a reference on the device until we're finished using it.  This is not
> >      an ideal fix because pcie_do_fatal_recovery() should not use the device at
> >      all after removing it, but that's too big a project for right now.
> >      #
> >      [bhelgaas: changelog, reduce get/put coverage]
> >      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> > index fdbcc555860d..674984a9277a 100644
> > --- a/drivers/pci/pcie/err.c
> > +++ b/drivers/pci/pcie/err.c
> > @@ -291,6 +291,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> >   	parent = udev->subordinate;
> >   	pci_lock_rescan_remove();
> > +	pci_dev_get(dev);
> >   	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
> >   					 bus_list) {
> >   		pci_dev_get(pdev);
> > @@ -325,6 +326,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
> >   		pci_info(dev, "Device recovery from fatal error failed\n");
> >   	}
> > +	pci_dev_put(dev);
> >   	pci_unlock_rescan_remove();
> >   }
> > 
> > > Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
> > > ---
> > >   drivers/pci/pcie/aer.c | 27 +++++++++++++++++++++++++--
> > >   1 file changed, 25 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > > index a2e8838..6e5e6a5 100644
> > > --- a/drivers/pci/pcie/aer.c
> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -657,6 +657,10 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
> > >   static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
> > >   {
> > >   	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
> > > +		/* increment reference count to keep the dev
> > > +		 * around until remove_source_device()
> > > +		 */
> > > +		pci_dev_get(dev);
> > >   		e_info->dev[e_info->error_dev_num] = dev;
> > >   		e_info->error_dev_num++;
> > >   		return 0;
> > > @@ -665,6 +669,21 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
> > >   }
> > >   /**
> > > + * remove_source_device -remove error devices from the e_info
> > > + * @e_info: pointer to error info
> > > + */
> > > +static void remove_source_device(struct aer_err_info *e_info)
> > > +{
> > > +	struct pci_dev *dev;
> > > +
> > > +	while (e_info->error_dev_num > 0) {
> > > +		e_info->error_dev_num--;
> > > +		dev = e_info->dev[e_info->error_dev_num];
> > > +		pci_dev_put(dev);
> > > +	}
> > > +}
> > > +
> > > +/**
> > >    * is_error_source - check whether the device is source of reported error
> > >    * @dev: pointer to pci_dev to be checked
> > >    * @e_info: pointer to reported error info
> > > @@ -976,8 +995,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
> > >   			e_info->multi_error_valid = 0;
> > >   		aer_print_port_info(pdev, e_info);
> > > -		if (find_source_device(pdev, e_info))
> > > +		if (find_source_device(pdev, e_info)) {
> > >   			aer_process_err_devices(e_info);
> > > +			remove_source_device(e_info);
> > > +		}
> > >   	}
> > >   	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
> > > @@ -995,8 +1016,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
> > >   		aer_print_port_info(pdev, e_info);
> > > -		if (find_source_device(pdev, e_info))
> > > +		if (find_source_device(pdev, e_info)) {
> > >   			aer_process_err_devices(e_info);
> > > +			remove_source_device(e_info);
> > > +		}
> > >   	}
> > >   }
> > > -- 
> > > 1.8.3.1
> > > 

Re: [PATCH V3, 1/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Thomas Tai]

On 07/26/2018 01:18 PM, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2018 at 10:29:18AM -0400, Thomas Tai wrote:
>> [ ... ]>
>>> I know I suggested this strategy, but I think this ended up being more
>>> complicated than it's worth.
>>>
>>> The problem code in pcie_do_fatal_recovery() essentially looks like
>>> this:
>>>
>>>     pcie_do_fatal_recovery(dev)
>>>       pci_stop_and_remove_bus_device(dev);
>>>       reset_link(dev);
>>>       pci_cleanup_aer_uncorrect_error_status(dev);
>>>       pcie_wait_for_link(dev, ...);
>>>       pci_uevent_ers(dev, ...);
>>>       pci_info(dev, ...);
>>>
>>> Some of this depends on the device type (bridge vs. endpoint) and the
>>> caller (AER vs. DPC), but given the right conditions, we can exercise
>>> all the above calls.
>>>
>>> I think it is just broken that we keep doing things with "dev" after
>>> removing it.  IMHO this code should be restructured to avoid that.
>>>
>>> I think fiddling with the refcount as in this patch adds too much
>>> complexity and makes it look like the current structure of
>>> pcie_do_fatal_recovery() is reasonable when it really isn't.
>>>
>>> But restructuring pcie_do_fatal_recovery() is too big a project to do
>>> before v4.18, and we need to fix this problem.  I propose that we
>>> merge your v2 patch for now, so at least the band-aid is in the
>>> function that I think is broken.
>>>
>>> I *would* like to reduce the scope of the get/put as in the patch
>>> below, though, so it is contained inside the rescan_remove lock.
>>> Could you try it and make sure it's still enough to avoid the problem?
>>> If it is, I'll add your sign-off and get this in v4.18.
>>
>> Hi Bjorn,
>> Thank you for your review and the details analysis. Sure, let's do the work
>> around for now. I retested your patch below and works fine. You are welcome
>> to add my signed-off and get this in v4.18.
> 
> OK, I added your signed-off-by and put the patch below on my for-linus
> branch for v4.18.

Cool. Thank you Bjorn.

> 
>> As far as reworking the pcie_do_fatal_recovery() goes, would you think I can
>> help out in any way? May be I can try rework the code to not use the dev
>> after it is removed.
> 
> That'd be great!  I expect Oza and Keith will have useful insight
> there, too, so keep them in the loop.

Sure, I will keep Oza and Keith in the loop too.

Thank you,
Thomas


> 
>>> commit 277ce38f2ed6a4310acf3bd541fb3aee4ec27dee
>>> Author: Thomas Tai <thomas.tai@oracle.com>
>>> Date:   Tue Jul 24 16:47:59 2018 -0500
>>>
>>>       PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()
>>>       When an fatal error is received by a non-bridge device, the device is
>>>       removed, and pci_stop_and_remove_bus_device() deallocates the device
>>>       structure.  The freed device structure is used by subsequent code to send
>>>       uevents and print messages.
>>>       Hold a reference on the device until we're finished using it.  This is not
>>>       an ideal fix because pcie_do_fatal_recovery() should not use the device at
>>>       all after removing it, but that's too big a project for right now.
>>>       #
>>>       [bhelgaas: changelog, reduce get/put coverage]
>>>       Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index fdbcc555860d..674984a9277a 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -291,6 +291,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>>>    	parent = udev->subordinate;
>>>    	pci_lock_rescan_remove();
>>> +	pci_dev_get(dev);
>>>    	list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>>>    					 bus_list) {
>>>    		pci_dev_get(pdev);
>>> @@ -325,6 +326,7 @@ void pcie_do_fatal_recovery(struct pci_dev *dev, u32 service)
>>>    		pci_info(dev, "Device recovery from fatal error failed\n");
>>>    	}
>>> +	pci_dev_put(dev);
>>>    	pci_unlock_rescan_remove();
>>>    }
>>>
>>>> Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
>>>> ---
>>>>    drivers/pci/pcie/aer.c | 27 +++++++++++++++++++++++++--
>>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index a2e8838..6e5e6a5 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -657,6 +657,10 @@ void cper_print_aer(struct pci_dev *dev, int aer_severity,
>>>>    static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>>>>    {
>>>>    	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
>>>> +		/* increment reference count to keep the dev
>>>> +		 * around until remove_source_device()
>>>> +		 */
>>>> +		pci_dev_get(dev);
>>>>    		e_info->dev[e_info->error_dev_num] = dev;
>>>>    		e_info->error_dev_num++;
>>>>    		return 0;
>>>> @@ -665,6 +669,21 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>>>>    }
>>>>    /**
>>>> + * remove_source_device -remove error devices from the e_info
>>>> + * @e_info: pointer to error info
>>>> + */
>>>> +static void remove_source_device(struct aer_err_info *e_info)
>>>> +{
>>>> +	struct pci_dev *dev;
>>>> +
>>>> +	while (e_info->error_dev_num > 0) {
>>>> +		e_info->error_dev_num--;
>>>> +		dev = e_info->dev[e_info->error_dev_num];
>>>> +		pci_dev_put(dev);
>>>> +	}
>>>> +}
>>>> +
>>>> +/**
>>>>     * is_error_source - check whether the device is source of reported error
>>>>     * @dev: pointer to pci_dev to be checked
>>>>     * @e_info: pointer to reported error info
>>>> @@ -976,8 +995,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>>>>    			e_info->multi_error_valid = 0;
>>>>    		aer_print_port_info(pdev, e_info);
>>>> -		if (find_source_device(pdev, e_info))
>>>> +		if (find_source_device(pdev, e_info)) {
>>>>    			aer_process_err_devices(e_info);
>>>> +			remove_source_device(e_info);
>>>> +		}
>>>>    	}
>>>>    	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
>>>> @@ -995,8 +1016,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>>>>    		aer_print_port_info(pdev, e_info);
>>>> -		if (find_source_device(pdev, e_info))
>>>> +		if (find_source_device(pdev, e_info)) {
>>>>    			aer_process_err_devices(e_info);
>>>> +			remove_source_device(e_info);
>>>> +		}
>>>>    	}
>>>>    }
>>>> -- 
>>>> 1.8.3.1
>>>>