x86/mce: Fix buggy error path in cpu_bank_alloc() leading to UAF

[PATCH] x86/mce: Fix buggy error path in cpu_bank_alloc() leading to UAF

Posted by Andrew Cooper 1 week, 3 days ago

When cleaning up from a mcabanks_alloc() failure, the memory is freed but
stale pointers are left in the percpu variables.

Use cpu_bank_free() which is idempotent and behaves correctly.

Fixes: 2e6c8f182c9c ("x86: distinguish CPU offlining from CPU removal")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

2e6c8f182c9c updated the success path but missed the associated error path.

Prior to that, the paths were at least consistent since their introduction in
commit 78c579426fb5 ("x86/MCE: Implement clearbank callback for AMD").
---
 xen/arch/x86/cpu/mcheck/mce.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 9a91807cfb33..684871b216a4 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -694,8 +694,7 @@ static int cpu_bank_alloc(unsigned int cpu)
 
     if ( !poll || !clr )
     {
-        mcabanks_free(poll);
-        mcabanks_free(clr);
+        cpu_bank_free(cpu);
         return -ENOMEM;
     }
 
-- 
2.39.5

Re: [PATCH] x86/mce: Fix buggy error path in cpu_bank_alloc() leading to UAF

Posted by Jan Beulich 1 week, 3 days ago

On 13.03.2026 11:49, Andrew Cooper wrote:
> When cleaning up from a mcabanks_alloc() failure, the memory is freed but
> stale pointers are left in the percpu variables.
> 
> Use cpu_bank_free() which is idempotent and behaves correctly.
> 
> Fixes: 2e6c8f182c9c ("x86: distinguish CPU offlining from CPU removal")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> 2e6c8f182c9c updated the success path but missed the associated error path.

I don't think so, see below.

> Prior to that, the paths were at least consistent since their introduction in
> commit 78c579426fb5 ("x86/MCE: Implement clearbank callback for AMD").
> ---
>  xen/arch/x86/cpu/mcheck/mce.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 9a91807cfb33..684871b216a4 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -694,8 +694,7 @@ static int cpu_bank_alloc(unsigned int cpu)
>  
>      if ( !poll || !clr )
>      {
> -        mcabanks_free(poll);
> -        mcabanks_free(clr);
> +        cpu_bank_free(cpu);

But this way we'll leak the allocation that may have succeeded, as the per-CPU
data is updated only afterwards. Yet it's those per-CPU items which
cpu_bank_free() reads.

I don't think there's any bug to fix here: Neither of the two pointers is
allocated anywhere else, so the per-CPU slots are always both NULL or both
non-NULL.

Jan