1. Patchew
  2. Xen
  3. [PATCH 00/10] x86/ucode: Fix module-handling use-aftere-free's
  4. View patch

[PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller

Andrew Cooper posted 10 patches 3 weeks, 3 days ago
[PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller
Posted by Andrew Cooper 3 weeks, 3 days ago 
No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/cpu/microcode/core.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 0b00e70f3b07..4c4003bf9687 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi)
     }
 }
 
-static void __init microcode_grab_module(struct boot_info *bi)
-{
-    if ( ucode_mod_idx < 0 )
-        ucode_mod_idx += bi->nr_modules;
-    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
-         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
-        goto scan;
-    ucode_mod = *bi->mods[ucode_mod_idx].mod;
-scan:
-    if ( ucode_scan )
-        microcode_scan_module(bi);
-}
-
 static struct microcode_ops __ro_after_init ucode_ops;
 
 static DEFINE_SPINLOCK(microcode_mutex);
@@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi)
     size_t size;
     struct microcode_patch *patch;
 
-    microcode_grab_module(bi);
+    if ( ucode_mod_idx < 0 )
+        ucode_mod_idx += bi->nr_modules;
+    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
+         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
+        goto scan;
+    ucode_mod = *bi->mods[ucode_mod_idx].mod;
+ scan:
+    if ( ucode_scan )
+        microcode_scan_module(bi);
 
     if ( !ucode_mod.mod_end && !ucode_blob.size )
         return 0;
-- 
2.39.5
Re: [PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller
Posted by Daniel P. Smith 2 weeks, 5 days ago 
On 10/28/24 05:18, Andrew Cooper wrote:
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>   xen/arch/x86/cpu/microcode/core.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index 0b00e70f3b07..4c4003bf9687 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi)
>       }
>   }
>   
> -static void __init microcode_grab_module(struct boot_info *bi)
> -{
> -    if ( ucode_mod_idx < 0 )
> -        ucode_mod_idx += bi->nr_modules;
> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> -         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> -        goto scan;
> -    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> -scan:
> -    if ( ucode_scan )
> -        microcode_scan_module(bi);
> -}
> -
>   static struct microcode_ops __ro_after_init ucode_ops;
>   
>   static DEFINE_SPINLOCK(microcode_mutex);
> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi)
>       size_t size;
>       struct microcode_patch *patch;
>   
> -    microcode_grab_module(bi);
> +    if ( ucode_mod_idx < 0 )
> +        ucode_mod_idx += bi->nr_modules;
> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> +         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> +        goto scan;
> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> + scan:
> +    if ( ucode_scan )
> +        microcode_scan_module(bi);
>   
>       if ( !ucode_mod.mod_end && !ucode_blob.size )
>           return 0;

Just a suggestion, but would it be useful to add a comment to explain 
the three conditions of ucode_mod_idx that are being handled before 
scanning is attempted? These being negative value, a valid module index, 
and module index wasn't already claimed in the module map.

Otherwise,

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Re: [PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller
Posted by Andrew Cooper 2 weeks, 1 day ago 
On 02/11/2024 4:33 pm, Daniel P. Smith wrote:
> On 10/28/24 05:18, Andrew Cooper wrote:
>> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct
>> boot_info *bi)
>>       size_t size;
>>       struct microcode_patch *patch;
>>   -    microcode_grab_module(bi);
>> +    if ( ucode_mod_idx < 0 )
>> +        ucode_mod_idx += bi->nr_modules;
>> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
>> +         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
>> +        goto scan;
>> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
>> + scan:
>> +    if ( ucode_scan )
>> +        microcode_scan_module(bi);
>>         if ( !ucode_mod.mod_end && !ucode_blob.size )
>>           return 0;
>
> Just a suggestion, but would it be useful to add a comment to explain
> the three conditions of ucode_mod_idx that are being handled before
> scanning is attempted?

The already did that, but it's in the next patch, which also rearranges
this block massively.

~Andrew
Re: [PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller
Posted by Daniel P. Smith 2 weeks, 1 day ago 
On 11/5/24 18:07, Andrew Cooper wrote:
> On 02/11/2024 4:33 pm, Daniel P. Smith wrote:
>> On 10/28/24 05:18, Andrew Cooper wrote:
>>> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct
>>> boot_info *bi)
>>>        size_t size;
>>>        struct microcode_patch *patch;
>>>    -    microcode_grab_module(bi);
>>> +    if ( ucode_mod_idx < 0 )
>>> +        ucode_mod_idx += bi->nr_modules;
>>> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
>>> +         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
>>> +        goto scan;
>>> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
>>> + scan:
>>> +    if ( ucode_scan )
>>> +        microcode_scan_module(bi);
>>>          if ( !ucode_mod.mod_end && !ucode_blob.size )
>>>            return 0;
>>
>> Just a suggestion, but would it be useful to add a comment to explain
>> the three conditions of ucode_mod_idx that are being handled before
>> scanning is attempted?
> 
> The already did that, but it's in the next patch, which also rearranges
> this block massively.

I realized that and meant to come back to adjust my response. As such,

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Re: [PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller
Posted by Jan Beulich 3 weeks, 3 days ago 
On 28.10.2024 10:18, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi)
>      }
>  }
>  
> -static void __init microcode_grab_module(struct boot_info *bi)
> -{
> -    if ( ucode_mod_idx < 0 )
> -        ucode_mod_idx += bi->nr_modules;
> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> -         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> -        goto scan;
> -    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> -scan:
> -    if ( ucode_scan )
> -        microcode_scan_module(bi);
> -}
> -
>  static struct microcode_ops __ro_after_init ucode_ops;
>  
>  static DEFINE_SPINLOCK(microcode_mutex);
> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi)
>      size_t size;
>      struct microcode_patch *patch;
>  
> -    microcode_grab_module(bi);
> +    if ( ucode_mod_idx < 0 )
> +        ucode_mod_idx += bi->nr_modules;
> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> +         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> +        goto scan;
> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> + scan:
> +    if ( ucode_scan )
> +        microcode_scan_module(bi);
>  
>      if ( !ucode_mod.mod_end && !ucode_blob.size )
>          return 0;

Could I talk you into doing away with the goto at the same time, by inverting
the if()'s condition? Preferably with that:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.