[PATCH v2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()

[PATCH v2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()

[Russell Currey]

eeh_handle_special_event() is called when an EEH event is detected but
can't be narrowed down to a specific PE.  This function looks through
every PE to find one in an erroneous state, then calls the regular event
handler eeh_handle_normal_event() once it knows which PE has an error.

However, if eeh_handle_normal_event() found that the PE cannot possibly
be recovered, it will free it, rendering the passed PE stale.
This leads to a use after free in eeh_handle_special_event() as it attempts to
clear the "recovering" state on the PE after eeh_handle_normal_event() returns.

Thus, make sure the PE is valid when attempting to clear state in
eeh_handle_special_event().

Cc: <stable@vger.kernel.org> #3.10+
Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
V2: check a specific return path instead of looking at the PE itself
---
 arch/powerpc/kernel/eeh_driver.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index b94887165a10..e510408e08e1 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -724,7 +724,7 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
  */
 #define MAX_WAIT_FOR_RECOVERY 300
 
-static void eeh_handle_normal_event(struct eeh_pe *pe)
+static int eeh_handle_normal_event(struct eeh_pe *pe)
 {
 	struct pci_bus *frozen_bus;
 	struct eeh_dev *edev, *tmp;
@@ -736,7 +736,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
 	if (!frozen_bus) {
 		pr_err("%s: Cannot find PCI bus for PHB#%x-PE#%x\n",
 			__func__, pe->phb->global_number, pe->addr);
-		return;
+		return -EIO;
 	}
 
 	eeh_pe_update_time_stamp(pe);
@@ -870,7 +870,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
 	pr_info("EEH: Notify device driver to resume\n");
 	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
 
-	return;
+	return rc;
 
 excess_failures:
 	/*
@@ -915,8 +915,12 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
 			pci_lock_rescan_remove();
 			pci_hp_remove_devices(frozen_bus);
 			pci_unlock_rescan_remove();
+
+			/* The passed PE should no longer be used */
+			rc = -EFAULT;
 		}
 	}
+	return rc;
 }
 
 static void eeh_handle_special_event(void)
@@ -982,7 +986,14 @@ static void eeh_handle_special_event(void)
 		 */
 		if (rc == EEH_NEXT_ERR_FROZEN_PE ||
 		    rc == EEH_NEXT_ERR_FENCED_PHB) {
-			eeh_handle_normal_event(pe);
+			/*
+			 * eeh_handle_normal_event() can make the PE stale if it
+			 * determines that the PE cannot possibly be recovered.
+			 * Don't modify the PE state if that's the case.
+			 */
+			if (eeh_handle_normal_event(pe) == -EFAULT)
+				continue;
+
 			eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
 		} else {
 			pci_lock_rescan_remove();
-- 
2.12.2

Re: [PATCH v2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()

[Andrew Donnellan]

On 10/04/17 17:11, Russell Currey wrote:
> eeh_handle_special_event() is called when an EEH event is detected but
> can't be narrowed down to a specific PE.  This function looks through
> every PE to find one in an erroneous state, then calls the regular event
> handler eeh_handle_normal_event() once it knows which PE has an error.
>
> However, if eeh_handle_normal_event() found that the PE cannot possibly
> be recovered, it will free it, rendering the passed PE stale.
> This leads to a use after free in eeh_handle_special_event() as it attempts to
> clear the "recovering" state on the PE after eeh_handle_normal_event() returns.
>
> Thus, make sure the PE is valid when attempting to clear state in
> eeh_handle_special_event().
>
> Cc: <stable@vger.kernel.org> #3.10+
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Russell Currey <ruscur@russell.cc>

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>


-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCH v2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()

[Alexey Kardashevskiy]

On 10/04/17 17:11, Russell Currey wrote:
> eeh_handle_special_event() is called when an EEH event is detected but
> can't be narrowed down to a specific PE.  This function looks through
> every PE to find one in an erroneous state, then calls the regular event
> handler eeh_handle_normal_event() once it knows which PE has an error.
> 
> However, if eeh_handle_normal_event() found that the PE cannot possibly
> be recovered, it will free it, rendering the passed PE stale.
> This leads to a use after free in eeh_handle_special_event() as it attempts to
> clear the "recovering" state on the PE after eeh_handle_normal_event() returns.
> 
> Thus, make sure the PE is valid when attempting to clear state in
> eeh_handle_special_event().
> 
> Cc: <stable@vger.kernel.org> #3.10+
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> V2: check a specific return path instead of looking at the PE itself
> ---
>  arch/powerpc/kernel/eeh_driver.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> index b94887165a10..e510408e08e1 100644
> --- a/arch/powerpc/kernel/eeh_driver.c
> +++ b/arch/powerpc/kernel/eeh_driver.c
> @@ -724,7 +724,7 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
>   */
>  #define MAX_WAIT_FOR_RECOVERY 300
>  
> -static void eeh_handle_normal_event(struct eeh_pe *pe)
> +static int eeh_handle_normal_event(struct eeh_pe *pe)
>  {
>  	struct pci_bus *frozen_bus;
>  	struct eeh_dev *edev, *tmp;
> @@ -736,7 +736,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
>  	if (!frozen_bus) {
>  		pr_err("%s: Cannot find PCI bus for PHB#%x-PE#%x\n",
>  			__func__, pe->phb->global_number, pe->addr);
> -		return;
> +		return -EIO;
>  	}
>  
>  	eeh_pe_update_time_stamp(pe);
> @@ -870,7 +870,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
>  	pr_info("EEH: Notify device driver to resume\n");
>  	eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
>  
> -	return;
> +	return rc;


eeh_handle_normal_event() uses "rc" to store return values from different
things:

- eeh_ops->wait_state() (which is pnv_eeh_wait_state) returns mask of
EEH_STATE_MMIO_ACTIVE/etc, errors would be EEH_STATE_UNAVAILABLE or
EEH_STATE_NOT_SUPPORT, both positive and the latter is used everywhere in
EEH code to report errors instead of negative linux errors.

- eeh_reset_device() returns usual linux negative errors, such as -EIO,
-ENODEV.

I'd suggest following one scheme or another here.


>  
>  excess_failures:
>  	/*
> @@ -915,8 +915,12 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
>  			pci_lock_rescan_remove();
>  			pci_hp_remove_devices(frozen_bus);
>  			pci_unlock_rescan_remove();
> +
> +			/* The passed PE should no longer be used */
> +			rc = -EFAULT;


I looked if eeh_handle_normal_event() could return -EFAULT in any other
branch. eeh_pci_enable() could return linux error if it failed in
pnv_eeh_set_option() which at least in theory can get -EFAULT from
unfreeze_pe(). Do we want to rely of the fact that unfreeze_pe() won't
return -EFAULT and we do not end up with non removed device?

May be instead of very popular EFAULT, eeh_handle_normal_event() is better
to return bool saying that pe is dead? Or move the whole
if(frozen_bus){
	...
	pci_hp_remove_devices()
	...
}

to a eeh_teardown_frozen_bus() helper, make eeh_handle_normal_event()
return frozen_bus if it needs to be frozen and pass that to
eeh_teardown_frozen_bus() if not NULL?


btw eeh_handle_normal_event() does not really need the excess_failures
label, that "excess_failures: pr_err()" could be moved to "goto
excess_failures" (which will become "goto perm_error") and then perm_error:
could go too :)


>  		}
>  	}
> +	return rc;
>  }
>  
>  static void eeh_handle_special_event(void)
> @@ -982,7 +986,14 @@ static void eeh_handle_special_event(void)
>  		 */
>  		if (rc == EEH_NEXT_ERR_FROZEN_PE ||
>  		    rc == EEH_NEXT_ERR_FENCED_PHB) {
> -			eeh_handle_normal_event(pe);
> +			/*
> +			 * eeh_handle_normal_event() can make the PE stale if it
> +			 * determines that the PE cannot possibly be recovered.
> +			 * Don't modify the PE state if that's the case.
> +			 */
> +			if (eeh_handle_normal_event(pe) == -EFAULT)
> +				continue;
> +
>  			eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>  		} else {
>  			pci_lock_rescan_remove();
> 


-- 
Alexey