On Mon, Sep 08, 2014 at 02:37:47PM -0300, Henrique de Moraes Holschuh wrote:
> The Intel SDM vol 3A, section 9.11.1, and also table 9-6, requires that
> the Data Size field be a multiple of 4 bytes. All of the microcode
> update header structures are dword-based, so the Total Size field must
> also be a multiple of the dword size.
>
> Ensure that data_size is a multiple of the dword size (4 bytes). The
> driver code assumes this to be true for both data_size and total_size,
> and will not work correctly otherwise.
>
> Futhermore, require that total_size be a multiple of 1024, as per the
> Intel SDM, vol 3A, section 9.11.1, page 9-28; table 9-6, page 9-29, and
> others. Test added by request of Borislav Petkov.
>
> Also refuse a microcode update with a microcode revision of zero.
> According to the Intel SDM, vol 3A, section 9.11.7, page 9-36, a
> microcode revision of zero is special:
>
> "CPUID returns a value in a model specific register in addition to
> its usual register return values. The semantics of CPUID cause it
> to deposit an update ID value in the 64-bit model-specific register
> at address 08BH (IA32_BIOS_SIGN_ID). If no update is present in the
> processor, the value in the MSR remains unmodified. The BIOS must
> pre-load a zero into the MSR before executing CPUID. If a read of
> the MSR at 8BH still returns zero after executing CPUID, this
> indicates that no update is present."
>
> This effectively reserves revision zero to mean "no microcode update
> installed on the processor": the microcode loader cannot differentiate
> sucess from failure when updating microcode to the same revision as the
> one currently installed on the processor, and this would always happen
> to updates to revision zero in the BIOS/UEFI loader.
>
> There is every reason to be paranoid about any microcode update with a
> revision of zero, as Intel will never release such a microcode update.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel_lib.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index ce69320..25915e3 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -55,9 +55,10 @@ int microcode_sanity_check(void *mc, int print_err)
> total_size = get_totalsize(mc_header);
> data_size = get_datasize(mc_header);
>
> - if (data_size + MC_HEADER_SIZE > total_size) {
> + if ((data_size % DWSIZE) || (total_size % 1024) ||
> + (data_size + MC_HEADER_SIZE > total_size)) {
> if (print_err)
> - pr_err("error! Bad data size in microcode data file\n");
> + pr_err("error: bad data size or total size in microcode data file\n");
Shorten:
pr_err("error: bad data/total size in microcode data file\n");
> return -EINVAL;
> }
>
> @@ -83,6 +84,26 @@ int microcode_sanity_check(void *mc, int print_err)
> ext_sigcount = ext_header->count;
> }
>
> + /*
> + * A version 1 loader cannot differentiate failure from success when
> + * attempting a microcode update to the same revision as the one
> + * currently installed. The loader is supposed to never attempt a
> + * same-version update (or a microcode downgrade, for that matter).
> + *
> + * This will always cause issues for microcode updates to revision zero
> + * in the UEFI/BIOS microcode loader: the processor reports a revision
> + * of zero when it is running without any microcode updates installed,
> + * such as after a reset/power up.
> + *
> + * Intel will never issue a microcode update with a revision of zero
> + * for the version 1 loader. Reject it.
> + */
This comment is too long. How about this instead:
/*
* 0 is not a valid microcode revision as it is used to denote the
* failure of a microcode update, see MSR 0x8b (IA32_BIOS_SIGN_ID):
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/
This is one of those seldom times where the documentation is actually clear. :-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Sun, 05 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:47PM -0300, Henrique de Moraes Holschuh wrote:
> > - if (data_size + MC_HEADER_SIZE > total_size) {
> > + if ((data_size % DWSIZE) || (total_size % 1024) ||
> > + (data_size + MC_HEADER_SIZE > total_size)) {
> > if (print_err)
> > - pr_err("error! Bad data size in microcode data file\n");
> > + pr_err("error: bad data size or total size in microcode data file\n");
>
> Shorten:
>
> pr_err("error: bad data/total size in microcode data file\n");
will do.
> > + /*
> > + * A version 1 loader cannot differentiate failure from success when
> > + * attempting a microcode update to the same revision as the one
> > + * currently installed. The loader is supposed to never attempt a
> > + * same-version update (or a microcode downgrade, for that matter).
> > + *
> > + * This will always cause issues for microcode updates to revision zero
> > + * in the UEFI/BIOS microcode loader: the processor reports a revision
> > + * of zero when it is running without any microcode updates installed,
> > + * such as after a reset/power up.
> > + *
> > + * Intel will never issue a microcode update with a revision of zero
> > + * for the version 1 loader. Reject it.
> > + */
>
> This comment is too long. How about this instead:
>
> /*
> * 0 is not a valid microcode revision as it is used to denote the
> * failure of a microcode update, see MSR 0x8b (IA32_BIOS_SIGN_ID):
> *
> * "It is required that this register field be pre-loaded with zero
> * prior to executing the CPUID, function 1. If the field remains
> * equal to zero, then there is no microcode update loaded. Another
> * non-zero value will be the signature."
> */
>
> This is one of those seldom times where the documentation is actually clear. :-)
Not realy, because it got you confused! :-)
Zero does not denote a failure to update microcode. What zero means, *when
you did the pre-load and issued a cpuid(1)*, is that the processor microcode
has not been updated since power-on/reset.
What flags a *sucessful* microcode update is a change on IA32_BIOS_SIGN_ID
(which must be read with the zero preload and cpuid(1) protocol).
If IA32_BIOS_SIGN_ID didn't change, the microcode update was rejected...
obviously, this only holds when you never attempt to update the microcode to
the same version the processor already had running.
And that's why we cannot detect whether a same-version update worked or not.
The reason Intel will never issue a microcode with revision zero is because
it cannot be safely applied by UEFI or BIOS at system power up: it would
look like a same-version update (IA32_BIOS_SIGN_ID would return zero before
the update, and would return zero after the update, whether it was applied
sucessfully or not).
And since Intel will never issue such microcode, we don't want to deal with
anything that claims to be a microcode update to revision zero.
IOW, this is a failure:
IA32_BIOS_SIGN_ID before the update is the same as IA32_BIOS_SIGN_ID after
the update attempt.
this is a sucessful update:
IA32_BIOS_SIGN_ID before the update is different from IA32_BIOS_SIGN_ID
after the update attempt.
In any case, you always need to do the zero-preload and cpuid(1) to read
IA32_BIOS_SIGN_ID.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, Oct 05, 2014 at 04:37:03PM -0300, Henrique de Moraes Holschuh wrote:
> Not realy, because it got you confused! :-)
No, it didn't get me confused - it got you confused that I'm confused.
You need to read the comment as a *whole*. The zero value is special
because it is *used* to *denote* a failure. You can use any other
invalid revision value for that matter.
Maybe "denote" was not precise enough - maybe it should say "0 is an
invalid microcode revision and is used to detect the failure of a
microcode update" or similar. Yeah, "detect" sounds better.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Sun, 05 Oct 2014, Borislav Petkov wrote:
> On Sun, Oct 05, 2014 at 04:37:03PM -0300, Henrique de Moraes Holschuh wrote:
> > Not realy, because it got you confused! :-)
>
> No, it didn't get me confused - it got you confused that I'm confused.
Indeed.
> You need to read the comment as a *whole*. The zero value is special
> because it is *used* to *denote* a failure. You can use any other
> invalid revision value for that matter.
Well, the new wording is confusing me... so maybe we can try a third time?
:-)
> Maybe "denote" was not precise enough - maybe it should say "0 is an
> invalid microcode revision and is used to detect the failure of a
> microcode update" or similar. Yeah, "detect" sounds better.
How about this:
/*
* 0 is not a valid microcode revision as it is used to detect the
* absence of any sucessful microcode update since reset /
* power-on, see MSR 0x8b (IA32_BIOS_SIGN_ID):
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/
?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Sun, Oct 05, 2014 at 06:49:08PM -0300, Henrique de Moraes Holschuh wrote:
> How about this:
>
> /*
> * 0 is not a valid microcode revision as it is used to detect the
> * absence of any sucessful microcode update since reset /
> * power-on, see MSR 0x8b (IA32_BIOS_SIGN_ID):
> *
> * "It is required that this register field be pre-loaded with zero
> * prior to executing the CPUID, function 1. If the field remains
> * equal to zero, then there is no microcode update loaded. Another
> * non-zero value will be the signature."
> */
>
Yep.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--