We should verify the indicated size of a patch in a microcode container
file (whether it does not exceed the PATCH_MAX_SIZE value) and also whether
this file is actually large enough to contain it in the late loader
verify_and_add_patch() function.
The early loader already does the PATCH_MAX_SIZE check in parse_container()
function.
Signed-off-by: Maciej S. Szmigiero <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index eba9e3c8aa17..096cb58a563f 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -473,7 +473,7 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
}
static unsigned int verify_patch_size(u8 family, u32 patch_size,
- unsigned int size)
+ size_t size)
{
u32 max_size;
@@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
break;
}
- if (patch_size > min_t(u32, size, max_size)) {
+ if (patch_size > min_t(size_t, size, max_size)) {
pr_err("patch size mismatch\n");
return 0;
}
@@ -609,7 +609,7 @@ static void cleanup(void)
* driver cannot continue functioning normally. In such cases, we tear
* down everything we've used up so far and exit.
*/
-static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
+static int verify_and_add_patch(u8 family, u8 *fw, size_t leftover)
{
struct microcode_header_amd *mc_hdr;
struct ucode_patch *patch;
@@ -617,7 +617,15 @@ static int verify_and_add_patch(u8 family, u8 *fw, unsigned int leftover)
u32 proc_fam;
u16 proc_id;
+ if (leftover < SECTION_HDR_SIZE + sizeof(*mc_hdr))
+ return leftover;
+
patch_size = *(u32 *)(fw + 4);
+ if (patch_size > PATCH_MAX_SIZE) {
+ pr_err("patch size %u too large\n", patch_size);
+ return -EINVAL;
+ }
+
crnt_size = patch_size + SECTION_HDR_SIZE;
mc_hdr = (struct microcode_header_amd *)(fw + SECTION_HDR_SIZE);
proc_id = mc_hdr->processor_rev_id;
On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
> break;
> }
>
> - if (patch_size > min_t(u32, size, max_size)) {
> + if (patch_size > min_t(size_t, size, max_size)) {
So I don't like this conversion to 8-byte-width size_t's. It is not
necessary. I'm pretty sure we can do fine with signed and unsigned ints.
For example, you can convert the size to signed int (if it hasn't been
converted yet) and check for < 0 and stop further processing. And so on...
Thx.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply. Srsly.
On 22.03.2018 17:11, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:08:17AM +0100, Maciej S. Szmigiero wrote:
>> @@ -505,7 +505,7 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
>> break;
>> }
>>
>> - if (patch_size > min_t(u32, size, max_size)) {
>> + if (patch_size > min_t(size_t, size, max_size)) {
>
> So I don't like this conversion to 8-byte-width size_t's. It is not
> necessary. I'm pretty sure we can do fine with signed and unsigned ints.
It is possible to keep verify_patch_size() unmodified (with unsigned int
and u32) but microcode container files >4GB in size then may be rejected,
even if they are technically valid (while a bit unrealistic) on 64-bit
kernels.
Thanks,
Maciej
On March 23, 2018 9:40:50 AM CDT, "Maciej S. Szmigiero" <[email protected]> wrote:
>It is possible to keep verify_patch_size() unmodified (with unsigned
>int
>and u32) but microcode container files >4GB in size then may be
>rejected,
If we ever have to support that - which I'm pretty sure we won't - more changes to the container format and parsing will be needed anyway.
Thx.
--
Sent from a small device: formatting sux and brevity is inevitable.