[PATCH 0/3] Minor cleanups in AMD microcode loader

[PATCH 0/3] Minor cleanups in AMD microcode loader

[Nikolay Borisov]

Here are 3 minor patches that somewhat simplify and streamline the code in the
AMD's microcode container parsing portion of the loader. No functional changes
intended.


Nikolay Borisov (3):
  x86/microcode/AMD: Return bool from find_blobs_in_containers()
  x86/microcode/AMD: Make __verify_patch_size() return bool
  x86/microcode/AMD: Remove bogus comment from parse_container()

 arch/x86/kernel/cpu/microcode/amd.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

--
2.34.1

[PATCH 1/3] x86/microcode/AMD: Return bool from find_blobs_in_containers()

[Nikolay Borisov]

Instead of open-coding the check for size/data move it inside the
function and make it return a boolean indicating whether data was found
or not. No functional changes.

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f63b051f25a0..9986cb85c951 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -554,7 +554,7 @@ static bool get_builtin_microcode(struct cpio_data *cp)
 	return false;
 }
 
-static void __init find_blobs_in_containers(struct cpio_data *ret)
+static bool __init find_blobs_in_containers(struct cpio_data *ret)
 {
 	struct cpio_data cp;
 
@@ -562,6 +562,7 @@ static void __init find_blobs_in_containers(struct cpio_data *ret)
 		cp = find_microcode_in_initrd(ucode_path);
 
 	*ret = cp;
+	return cp.data && cp.size;
 }
 
 void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_eax)
@@ -576,8 +577,7 @@ void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_
 	/* Needed in load_microcode_amd() */
 	ucode_cpu_info[0].cpu_sig.sig = cpuid_1_eax;
 
-	find_blobs_in_containers(&cp);
-	if (!(cp.data && cp.size))
+	if (!find_blobs_in_containers(&cp))
 		return;
 
 	if (early_apply_microcode(ed->old_rev, cp.data, cp.size))
@@ -597,8 +597,7 @@ static int __init save_microcode_in_initrd(void)
 	if (dis_ucode_ldr || c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10)
 		return 0;
 
-	find_blobs_in_containers(&cp);
-	if (!(cp.data && cp.size))
+	if (!find_blobs_in_containers(&cp))
 		return -EINVAL;
 
 	scan_containers(cp.data, cp.size, &desc);
-- 
2.34.1

Re: [PATCH 1/3] x86/microcode/AMD: Return bool from find_blobs_in_containers()

[Borislav Petkov]

On Fri, Oct 18, 2024 at 06:51:49PM +0300, Nikolay Borisov wrote:
> @@ -562,6 +562,7 @@ static void __init find_blobs_in_containers(struct cpio_data *ret)
>  		cp = find_microcode_in_initrd(ucode_path);
>  
>  	*ret = cp;
> +	return cp.data && cp.size;

I guess we want this here ontop:

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 41b6f8a9e7e6..dfad4b26a662 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -561,12 +561,16 @@ static bool get_builtin_microcode(struct cpio_data *cp)
 static bool __init find_blobs_in_containers(struct cpio_data *ret)
 {
 	struct cpio_data cp;
+	bool found;
 
 	if (!get_builtin_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path);
 
-	*ret = cp;
-	return cp.data && cp.size;
+	found = cp.data && cp.size;
+	if (found)
+		*ret = cp;
+
+	return found;
 }
 
 void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_eax)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/microcode] x86/microcode/AMD: Return bool from find_blobs_in_containers()

[tip-bot2 for Nikolay Borisov]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     a85c08aaa665b5436d325f6d7138732a0e1315ce
Gitweb:        https://git.kernel.org/tip/a85c08aaa665b5436d325f6d7138732a0e1315ce
Author:        Nikolay Borisov <nik.borisov@suse.com>
AuthorDate:    Fri, 18 Oct 2024 18:51:49 +03:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 14:03:30 +01:00

x86/microcode/AMD: Return bool from find_blobs_in_containers()

Instead of open-coding the check for size/data move it inside the
function and make it return a boolean indicating whether data was found
or not.

No functional changes.

  [ bp: Write @ret in find_blobs_in_containers() only on success. ]

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20241018155151.702350-2-nik.borisov@suse.com
---
 arch/x86/kernel/cpu/microcode/amd.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index fb5d0c6..d395665 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -569,14 +569,19 @@ static bool get_builtin_microcode(struct cpio_data *cp)
 	return false;
 }
 
-static void __init find_blobs_in_containers(struct cpio_data *ret)
+static bool __init find_blobs_in_containers(struct cpio_data *ret)
 {
 	struct cpio_data cp;
+	bool found;
 
 	if (!get_builtin_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path);
 
-	*ret = cp;
+	found = cp.data && cp.size;
+	if (found)
+		*ret = cp;
+
+	return found;
 }
 
 void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_eax)
@@ -591,8 +596,7 @@ void __init load_ucode_amd_bsp(struct early_load_data *ed, unsigned int cpuid_1_
 	/* Needed in load_microcode_amd() */
 	ucode_cpu_info[0].cpu_sig.sig = cpuid_1_eax;
 
-	find_blobs_in_containers(&cp);
-	if (!(cp.data && cp.size))
+	if (!find_blobs_in_containers(&cp))
 		return;
 
 	if (early_apply_microcode(ed->old_rev, cp.data, cp.size))
@@ -612,8 +616,7 @@ static int __init save_microcode_in_initrd(void)
 	if (dis_ucode_ldr || c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10)
 		return 0;
 
-	find_blobs_in_containers(&cp);
-	if (!(cp.data && cp.size))
+	if (!find_blobs_in_containers(&cp))
 		return -EINVAL;
 
 	scan_containers(cp.data, cp.size, &desc);

[PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Nikolay Borisov]

The result of that function is in essence boolean, so simplify to return
the result of the relevant expression. It also makes it follow the
convetion used by __verify_patch_section(). No functional changes.

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 9986cb85c951..37a428b109a2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -282,7 +282,7 @@ __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize)
  * exceed the per-family maximum). @sh_psize is the size read from the section
  * header.
  */
-static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
+static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
 {
 	u8 family = x86_family(bsp_cpuid_1_eax);
 	u32 max_size;
@@ -305,10 +305,7 @@ static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
 		return 0;
 	}
 
-	if (sh_psize > min_t(u32, buf_size, max_size))
-		return 0;
-
-	return sh_psize;
+	return sh_psize <= min_t(u32, buf_size, max_size);
 }
 
 /*
@@ -323,7 +320,6 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 {
 	u8 family = x86_family(bsp_cpuid_1_eax);
 	struct microcode_header_amd *mc_hdr;
-	unsigned int ret;
 	u32 sh_psize;
 	u16 proc_id;
 	u8 patch_fam;
@@ -347,8 +343,7 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 		return -1;
 	}
 
-	ret = __verify_patch_size(sh_psize, buf_size);
-	if (!ret) {
+	if (!__verify_patch_size(sh_psize, buf_size)) {
 		pr_debug("Per-family patch size mismatch.\n");
 		return -1;
 	}
-- 
2.34.1

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Borislav Petkov]

On Fri, Oct 18, 2024 at 06:51:50PM +0300, Nikolay Borisov wrote:
> The result of that function is in essence boolean, so simplify to return
> the result of the relevant expression. It also makes it follow the
> convetion used by __verify_patch_section(). No functional changes.

convetion used by __verify_patch_section(). No functional changes.
Unknown word [convetion] in commit message.
Suggestions: ['convection', 'convention', 'conversion', 'confection', 'conviction', 'connection', 'confession']

You need a spellchecker. :)

> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 9986cb85c951..37a428b109a2 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -282,7 +282,7 @@ __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize)
>   * exceed the per-family maximum). @sh_psize is the size read from the section
>   * header.
>   */
> -static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
> +static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>  {
>  	u8 family = x86_family(bsp_cpuid_1_eax);
>  	u32 max_size;

You missed a spot here for the >= 0x15 families. And I think this is more
readable and more precise what is supposed to be checked here:

---
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 8bd79ad63437..0211c62bc4c4 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -289,7 +289,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
 	u32 max_size;
 
 	if (family >= 0x15)
-		return min_t(u32, sh_psize, buf_size);
+		return sh_psize == min_t(u32, sh_psize, buf_size);
 
 #define F1XH_MPB_MAX_SIZE 2048
 #define F14H_MPB_MAX_SIZE 1824
@@ -306,7 +306,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
 		return 0;
 	}
 
-	return sh_psize <= min_t(u32, buf_size, max_size);
+	return sh_psize == min_t(u32, buf_size, max_size);
 }
 
 /*

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Nikolay Borisov]

On 14.11.24 г. 14:58 ч., Borislav Petkov wrote:
> On Fri, Oct 18, 2024 at 06:51:50PM +0300, Nikolay Borisov wrote:
>> The result of that function is in essence boolean, so simplify to return
>> the result of the relevant expression. It also makes it follow the
>> convetion used by __verify_patch_section(). No functional changes.
> 
> convetion used by __verify_patch_section(). No functional changes.
> Unknown word [convetion] in commit message.
> Suggestions: ['convection', 'convention', 'conversion', 'confection', 'conviction', 'connection', 'confession']
> 
> You need a spellchecker. :)
> 
>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>> ---
>>   arch/x86/kernel/cpu/microcode/amd.c | 11 +++--------
>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
>> index 9986cb85c951..37a428b109a2 100644
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -282,7 +282,7 @@ __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize)
>>    * exceed the per-family maximum). @sh_psize is the size read from the section
>>    * header.
>>    */
>> -static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
>> +static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>>   {
>>   	u8 family = x86_family(bsp_cpuid_1_eax);
>>   	u32 max_size;
> 
> You missed a spot here for the >= 0x15 families. And I think this is more
> readable and more precise what is supposed to be checked here:
> 
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 8bd79ad63437..0211c62bc4c4 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -289,7 +289,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>   	u32 max_size;
>   
>   	if (family >= 0x15)
> -		return min_t(u32, sh_psize, buf_size);
> +		return sh_psize == min_t(u32, sh_psize, buf_size);

Indee.

>   
>   #define F1XH_MPB_MAX_SIZE 2048
>   #define F14H_MPB_MAX_SIZE 1824
> @@ -306,7 +306,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>   		return 0;
>   	}
>   
> -	return sh_psize <= min_t(u32, buf_size, max_size);
> +	return sh_psize == min_t(u32, buf_size, max_size);

For the older families we have a hard upper bound so we want to ensure 
that the size in the header is strictly <= than buf_size, which in turn 
must be <= max_size .


i.e Is it not valid to have sh_psize < buf_size rather than strictly equal ?

>   }
>   
>   /*
> 

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Borislav Petkov]

On Thu, Nov 14, 2024 at 03:19:33PM +0200, Nikolay Borisov wrote:
> For the older families we have a hard upper bound so we want to ensure that
> the size in the header is strictly <= than buf_size, which in turn must be
> <= max_size .
> 
> 
> i.e Is it not valid to have sh_psize < buf_size rather than strictly equal ?

Let's look at all possible cases:

* sh_psize > min_t(sh_psize, buf_size) == buf_size -- means the buffer is
truncated so the patch is incomplete 

* sh_psize < min_t(sh_psize, buf_size) == buf_size -- this is actually ok
because we're working with the whole buffer and there can be other patches
following. Now I remember why I had ">" there.

* sh_psize > min_t(u32, buf_size, max_size) == buf_size -- truncated buffer

* sh_psize < min_t(u32, buf_size, max_size) == buf_size -- that's ok

* sh_psize > min_t(u32, buf_size, max_size) == max_size -- some mismatch, fail

* sh_psize < min_t(u32, buf_size, max_size) == max_size -- ditto.

So this needs more staring and I need to make it more readable.

Btw, one more spot:

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 01ea25f31c0c..7554d83f00e6 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -303,7 +303,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
 		break;
 	default:
 		WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
-		return 0;
+		return false;
 	}
 
 	return sh_psize == min_t(u32, buf_size, max_size);

---

IOW, I'm thinking about something like this (pasting the whole function here):

static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
{
        u8 family = x86_family(bsp_cpuid_1_eax);
        u32 max_size;

        if (family >= 0x15)
                goto ret;

#define F1XH_MPB_MAX_SIZE 2048
#define F14H_MPB_MAX_SIZE 1824

        switch (family) {
        case 0x10 ... 0x12:
                max_size = F1XH_MPB_MAX_SIZE;
                break;
        case 0x14:
                max_size = F14H_MPB_MAX_SIZE;
                break;
        default:
                WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
                return false;
        }       

        if (sh_psize != max_size)
                return false;
                
ret:    
        /* Working with the whole buffer so < is ok. */
        return sh_psize <= buf_size;
}

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Nikolay Borisov]

On 14.11.24 г. 16:01 ч., Borislav Petkov wrote:
> On Thu, Nov 14, 2024 at 03:19:33PM +0200, Nikolay Borisov wrote:
>> For the older families we have a hard upper bound so we want to ensure that
>> the size in the header is strictly <= than buf_size, which in turn must be
>> <= max_size .
>>
>>
>> i.e Is it not valid to have sh_psize < buf_size rather than strictly equal ?
> 
> Let's look at all possible cases:
> 
> * sh_psize > min_t(sh_psize, buf_size) == buf_size -- means the buffer is
> truncated so the patch is incomplete
> 
> * sh_psize < min_t(sh_psize, buf_size) == buf_size -- this is actually ok
> because we're working with the whole buffer and there can be other patches
> following. Now I remember why I had ">" there.
> 
> * sh_psize > min_t(u32, buf_size, max_size) == buf_size -- truncated buffer
> 
> * sh_psize < min_t(u32, buf_size, max_size) == buf_size -- that's ok
> 
> * sh_psize > min_t(u32, buf_size, max_size) == max_size -- some mismatch, fail
> 
> * sh_psize < min_t(u32, buf_size, max_size) == max_size -- ditto.
> 
> So this needs more staring and I need to make it more readable.
> 
> Btw, one more spot:
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 01ea25f31c0c..7554d83f00e6 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -303,7 +303,7 @@ static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
>   		break;
>   	default:
>   		WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
> -		return 0;
> +		return false;
>   	}
>   
>   	return sh_psize == min_t(u32, buf_size, max_size);
> 
> ---
> 
> IOW, I'm thinking about something like this (pasting the whole function here):
> 
> static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
> {
>          u8 family = x86_family(bsp_cpuid_1_eax);
>          u32 max_size;
> 
>          if (family >= 0x15)
>                  goto ret;
> 
> #define F1XH_MPB_MAX_SIZE 2048
> #define F14H_MPB_MAX_SIZE 1824
> 
>          switch (family) {
>          case 0x10 ... 0x12:
>                  max_size = F1XH_MPB_MAX_SIZE;
>                  break;
>          case 0x14:
>                  max_size = F14H_MPB_MAX_SIZE;
>                  break;
>          default:
>                  WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
>                  return false;
>          }
> 
>          if (sh_psize != max_size)
>                  return false;

Isn't sh_psize < max_size valid here?

>                  
> ret:
>          /* Working with the whole buffer so < is ok. */
>          return sh_psize <= buf_size;
> }
> 

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Borislav Petkov]

On Thu, Nov 14, 2024 at 04:13:33PM +0200, Nikolay Borisov wrote:
> >          if (sh_psize != max_size)
> >                  return false;
> 
> Isn't sh_psize < max_size valid here?

* sh_psize < min_t(u32, buf_size, max_size) == max_size -- ditto.

This is still some sort of a mismatch which we'd rather fail.

That max_size should probably be called patch_size or so.

IOW, if the patch size in the header doesn't match the per-family patch size
=> fail.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Nikolay Borisov]

On 14.11.24 г. 16:26 ч., Borislav Petkov wrote:
> On Thu, Nov 14, 2024 at 04:13:33PM +0200, Nikolay Borisov wrote:
>>>           if (sh_psize != max_size)
>>>                   return false;
>>
>> Isn't sh_psize < max_size valid here?
> 
> * sh_psize < min_t(u32, buf_size, max_size) == max_size -- ditto.
> 
> This is still some sort of a mismatch which we'd rather fail.
> 
> That max_size should probably be called patch_size or so.
> 
> IOW, if the patch size in the header doesn't match the per-family patch size
> => fail.

Right, the important bit here is that max_size is not really max_size 
but, as you say, patch_size so for those families it's expected to have 
an exact size.

With max_size I perceive it would imply that the current patch can be 
_at most_ max_size, but might as well be smaller.



> 

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Borislav Petkov]

On Thu, Nov 14, 2024 at 04:40:50PM +0200, Nikolay Borisov wrote:
> Right, the important bit here is that max_size is not really max_size but,

I take that back and this really is max_size. I went back and looked. These
are the patches for the older families:

Patch 00: type 1, size:  960
Patch 01: type 1, size:  960
Patch 02: type 1, size:  960
Patch 03: type 1, size:  960
Patch 04: type 1, size:  960
Patch 05: type 1, size:  960
Patch 06: type 1, size:  960
Patch 07: type 1, size:  960
Patch 08: type 1, size:  512
Patch 09: type 1, size:  960
Patch 10: type 1, size: 1568
Patch 11: type 1, size: 1568

Lemme go and look in detail again, just to be sure.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/microcode/AMD: Make __verify_patch_size() return bool

[Borislav Petkov]

On Thu, Nov 14, 2024 at 04:47:04PM +0100, Borislav Petkov wrote:
> On Thu, Nov 14, 2024 at 04:40:50PM +0200, Nikolay Borisov wrote:
> > Right, the important bit here is that max_size is not really max_size but,
> 
> I take that back and this really is max_size. I went back and looked. These
> are the patches for the older families:
> 
> Patch 00: type 1, size:  960
> Patch 01: type 1, size:  960
> Patch 02: type 1, size:  960
> Patch 03: type 1, size:  960
> Patch 04: type 1, size:  960
> Patch 05: type 1, size:  960
> Patch 06: type 1, size:  960
> Patch 07: type 1, size:  960
> Patch 08: type 1, size:  512
> Patch 09: type 1, size:  960
> Patch 10: type 1, size: 1568
> Patch 11: type 1, size: 1568
> 
> Lemme go and look in detail again, just to be sure.

IOW the below. Which is basically equivalent to what we have now but converted
to return bool. Oh well.

static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
{
        u8 family = x86_family(bsp_cpuid_1_eax);
        u32 max_size;

        if (family >= 0x15)
                goto ret;

#define F1XH_MPB_MAX_SIZE 2048
#define F14H_MPB_MAX_SIZE 1824

        switch (family) {
        case 0x10 ... 0x12:
                max_size = F1XH_MPB_MAX_SIZE;
                break;
        case 0x14:
                max_size = F14H_MPB_MAX_SIZE;
                break;
        default:
                WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
                return false;
        }

        if (sh_psize > max_size)
                return false;

ret:
        /* Working with the whole buffer so < is ok. */
        return sh_psize <= buf_size;
}

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/microcode] x86/microcode/AMD: Make __verify_patch_size() return bool

[tip-bot2 for Nikolay Borisov]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     d8317f3d8e6b412ff51ea66f1de2b2f89835f811
Gitweb:        https://git.kernel.org/tip/d8317f3d8e6b412ff51ea66f1de2b2f89835f811
Author:        Nikolay Borisov <nik.borisov@suse.com>
AuthorDate:    Fri, 18 Oct 2024 18:51:50 +03:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 14:03:37 +01:00

x86/microcode/AMD: Make __verify_patch_size() return bool

The result of that function is in essence boolean, so simplify to return the
result of the relevant expression. It also makes it follow the convention used
by __verify_patch_section().

No functional changes.

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20241018155151.702350-3-nik.borisov@suse.com
---
 arch/x86/kernel/cpu/microcode/amd.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 95431e4..9a5ebbb 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -283,13 +283,13 @@ __verify_patch_section(const u8 *buf, size_t buf_size, u32 *sh_psize)
  * exceed the per-family maximum). @sh_psize is the size read from the section
  * header.
  */
-static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
+static bool __verify_patch_size(u32 sh_psize, size_t buf_size)
 {
 	u8 family = x86_family(bsp_cpuid_1_eax);
 	u32 max_size;
 
 	if (family >= 0x15)
-		return min_t(u32, sh_psize, buf_size);
+		goto ret;
 
 #define F1XH_MPB_MAX_SIZE 2048
 #define F14H_MPB_MAX_SIZE 1824
@@ -303,13 +303,15 @@ static unsigned int __verify_patch_size(u32 sh_psize, size_t buf_size)
 		break;
 	default:
 		WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
-		return 0;
+		return false;
 	}
 
-	if (sh_psize > min_t(u32, buf_size, max_size))
-		return 0;
+	if (sh_psize > max_size)
+		return false;
 
-	return sh_psize;
+ret:
+	/* Working with the whole buffer so < is ok. */
+	return sh_psize <= buf_size;
 }
 
 /*
@@ -324,7 +326,6 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 {
 	u8 family = x86_family(bsp_cpuid_1_eax);
 	struct microcode_header_amd *mc_hdr;
-	unsigned int ret;
 	u32 sh_psize;
 	u16 proc_id;
 	u8 patch_fam;
@@ -348,8 +349,7 @@ static int verify_patch(const u8 *buf, size_t buf_size, u32 *patch_size)
 		return -1;
 	}
 
-	ret = __verify_patch_size(sh_psize, buf_size);
-	if (!ret) {
+	if (!__verify_patch_size(sh_psize, buf_size)) {
 		pr_debug("Per-family patch size mismatch.\n");
 		return -1;
 	}

[PATCH 3/3] x86/microcode/AMD: Remove bogus comment from parse_container()

[Nikolay Borisov]

The functions doesn't return an equivalence ID, remove the false
comment.

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
 arch/x86/kernel/cpu/microcode/amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 37a428b109a2..f4e6d1f96b6b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -375,8 +375,8 @@ static bool mc_patch_matches(struct microcode_amd *mc, u16 eq_id)
 
 /*
  * This scans the ucode blob for the proper container as we can have multiple
- * containers glued together. Returns the equivalence ID from the equivalence
- * table or 0 if none found.
+ * containers glued together.
+ *
  * Returns the amount of bytes consumed while scanning. @desc contains all the
  * data we're going to use in later stages of the application.
  */
-- 
2.34.1

[tip: x86/microcode] x86/microcode/AMD: Remove bogus comment from parse_container()

[tip-bot2 for Nikolay Borisov]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     db80b2efa0377bf6e7d422fd7e6605481b3a0ee4
Gitweb:        https://git.kernel.org/tip/db80b2efa0377bf6e7d422fd7e6605481b3a0ee4
Author:        Nikolay Borisov <nik.borisov@suse.com>
AuthorDate:    Fri, 18 Oct 2024 18:51:51 +03:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 31 Dec 2024 14:03:33 +01:00

x86/microcode/AMD: Remove bogus comment from parse_container()

The function doesn't return an equivalence ID, remove the false comment.

Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20241018155151.702350-4-nik.borisov@suse.com
---
 arch/x86/kernel/cpu/microcode/amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index d395665..95431e4 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -381,8 +381,8 @@ static bool mc_patch_matches(struct microcode_amd *mc, u16 eq_id)
 
 /*
  * This scans the ucode blob for the proper container as we can have multiple
- * containers glued together. Returns the equivalence ID from the equivalence
- * table or 0 if none found.
+ * containers glued together.
+ *
  * Returns the amount of bytes consumed while scanning. @desc contains all the
  * data we're going to use in later stages of the application.
  */