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.1On 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
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 ?
> }
>
> /*
>
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
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;
> }
>
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
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. >
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
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
© 2016 - 2024 Red Hat, Inc.