Re: [LinuxBIOS] SMM is evil?

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [060511 22:06]:
> Stefan Reinauer wrote:
> > If something seems as simple as setting the D_LCK bit of SMM, we should
> > definitely do it.. It will at least be a marketable feature against
> > other upcoming firmware implementations.
>
> I believe that setting D_LCK will mitigate a few attacks but I strongly
> doubt that it cannot be cleared during system operation. Yes, the manual
> specifies it, but manuals have been underspecified before. Since we don't
> use SMM for anything, we might as well

While the mechanism might indeed provide some protection its maybe
possible to assert a reset signal to the chipset while not dropping out
of the code path, as a possible workaround. No idea whether this is
possible without "bios support" though.
> So, by all means, do it now. Until somebody figures out a way to disable
> D_LCK again we offer a much higher degree of security than everybody
> else.

Ok, D_LCK/D_OPEN/D_CLOSE is intel vocabulary. There is no such thing on
AMD. They call it SMMLOCK in their BKDG:

6.11.6 Locking SMM

The SMM registers can be locked by setting the SMMLOCK (HWCR, bit 0).
Once set, the SMM_BASE, the SMM_ADDR, all but the two close bits of
SMM_MASK and the RSMSPCYCDIS, SMISPCYCDIS, and SMMLOCK bits of HWCR are
locked and cannot be changed. The only way to unlock the SMM registers
is to assert reset.  This provides security to the SMM mechanism. The
BIOS can lock the SMM environment after setting it up so that it can not
be tampered with.

So I propose the following patch for LinuxBIOS to fix the SMM problem
for all supported AMD K8 mainboards:

Index: src/cpu/amd/model_fxx/model_fxx_init.c
===================================================================
--- src/cpu/amd/model_fxx/model_fxx_init.c      (revision 2302)
+++ src/cpu/amd/model_fxx/model_fxx_init.c      (working copy)
@@ -454,6 +454,12 @@

k8_errata();

+       /* Set SMMLOCK to avoid exploits messing with SMM */
+       msr = rdmsr(HWCR_MSR);
+       msr.lo |= (1 << 0);
+       wrmsr(HWCR_MSR, msr);
+
+
        enable_cache();

/* Enable the local cpu apics */

Anyone opposing?

Stefan