The maximum possible value returned by install_equiv_cpu_table() is
UINT_MAX + CONTAINER_HDR_SZ (on a 64-bit kernel).
This is more than (signed) int type currently returned by this function can
hold so this function will need to return a size_t instead.
The individual (negative) error codes returned by this function are of no
use anyway, since they are all normalized to UCODE_ERROR by its caller
(__load_microcode_amd()).
Signed-off-by: Maciej S. Szmigiero <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index ac06e2819f26..d20c327c960b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -547,37 +547,38 @@ static enum ucode_state apply_microcode_amd(int cpu)
return UCODE_UPDATED;
}
-static int install_equiv_cpu_table(const u8 *buf, size_t buf_size)
+static size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size)
{
unsigned int *ibuf = (unsigned int *)buf;
- unsigned int type, size;
+ unsigned int type;
+ size_t size;
if (buf_size < CONTAINER_HDR_SZ) {
pr_err("no container header\n");
- return -EINVAL;
+ return 0;
}
type = ibuf[1];
if (type != UCODE_EQUIV_CPU_TABLE_TYPE) {
pr_err("invalid type field in container file section header\n");
- return -EINVAL;
+ return 0;
}
size = ibuf[2];
if (size < sizeof(struct equiv_cpu_entry)) {
pr_err("equivalent CPU table too short\n");
- return -EINVAL;
+ return 0;
}
if (buf_size - CONTAINER_HDR_SZ < size) {
pr_err("equivalent CPU table truncated\n");
- return -EINVAL;
+ return 0;
}
equiv_cpu_table = vmalloc(size);
if (!equiv_cpu_table) {
pr_err("failed to allocate equivalent CPU table\n");
- return -ENOMEM;
+ return 0;
}
memcpy(equiv_cpu_table, buf + CONTAINER_HDR_SZ, size);
@@ -672,13 +673,13 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
size_t size)
{
enum ucode_state ret = UCODE_ERROR;
- unsigned int leftover;
+ size_t leftover;
u8 *fw = (u8 *)data;
int crnt_size = 0;
- int offset;
+ size_t offset;
offset = install_equiv_cpu_table(data, size);
- if (offset < 0) {
+ if (!offset) {
pr_err("failed to create equivalent cpu table\n");
return ret;
}
On Tue, Mar 13, 2018 at 10:06:34PM +0100, Maciej S. Szmigiero wrote:
> The maximum possible value returned by install_equiv_cpu_table() is
> UINT_MAX + CONTAINER_HDR_SZ (on a 64-bit kernel).
> This is more than (signed) int type currently returned by this function can
> hold so this function will need to return a size_t instead.
I'm trying to parse this but I'm not really sure.
All I know is:
unsigned int size = ibuf[2];
and that is really a 4-byte unsigned quantity so anything less is an
arbitrary limitation.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 14.03.2018 18:58, Borislav Petkov wrote:
> On Tue, Mar 13, 2018 at 10:06:34PM +0100, Maciej S. Szmigiero wrote:
>> The maximum possible value returned by install_equiv_cpu_table() is
>> UINT_MAX + CONTAINER_HDR_SZ (on a 64-bit kernel).
>> This is more than (signed) int type currently returned by this function can
>> hold so this function will need to return a size_t instead.
>
> I'm trying to parse this but I'm not really sure.
>
> All I know is:
>
> unsigned int size = ibuf[2];
>
> and that is really a 4-byte unsigned quantity so anything less is an
> arbitrary limitation.
There is no limit on CPU equivalence table length in this patch series
like it was in the previous version.
The maximum possible value returned by install_equiv_cpu_table() of
UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
This won't fit in 'int' type, hence this patch.
That's because this functions tells its caller how much bytes to skip
from the beginning of a microcode container file to the first patch
section contained in it.
Maciej
On Thu, Mar 15, 2018 at 12:46:05AM +0100, Maciej S. Szmigiero wrote:
> The maximum possible value returned by install_equiv_cpu_table() of
> UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
> variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
> This won't fit in 'int' type, hence this patch.
So make it fit by returning an unsigned int.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 15.03.2018 00:58, Borislav Petkov wrote:
> On Thu, Mar 15, 2018 at 12:46:05AM +0100, Maciej S. Szmigiero wrote:
>> The maximum possible value returned by install_equiv_cpu_table() of
>> UINT_MAX + CONTAINER_HDR_SZ comes from the maximum value of this 'size'
>> variable (that is UINT_MAX) plus the header length of CONTAINER_HDR_SZ.
>> This won't fit in 'int' type, hence this patch.
>
> So make it fit by returning an unsigned int.
>
This can be done if this function is modified to return only the CPU
equivalence table length (without the container header length), leaving
its single caller the job of adding the container header length to skip
to the fist patch section.
Otherwise we introduce a equivalence table length limit of
UINT_MAX - CONTAINER_HDR_SZ, as anything more will overflow an
unsigned int variable on a 64-bit kernel (on 32-bit this will be caught
by the equivalence table truncation check).
Maciej
On Thu, Mar 15, 2018 at 01:13:07AM +0100, Maciej S. Szmigiero wrote:
> This can be done if this function is modified to return only the CPU
> equivalence table length (without the container header length), leaving
> its single caller the job of adding the container header length to skip
> to the fist patch section.
Sure, it leaves the function to deal with the equiv table length only
and the caller then adds the header length. Which is actually cleaner.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 15.03.2018 01:56, Borislav Petkov wrote:
> On Thu, Mar 15, 2018 at 01:13:07AM +0100, Maciej S. Szmigiero wrote:
>> This can be done if this function is modified to return only the CPU
>> equivalence table length (without the container header length), leaving
>> its single caller the job of adding the container header length to skip
>> to the fist patch section.
>
> Sure, it leaves the function to deal with the equiv table length only
> and the caller then adds the header length. Which is actually cleaner.
>
OK, will do then.
Maciej