Re: [PATCH] x86: also disable FSRM if ERMS is disabled

[Daniel Verkamp]

On Fri, Sep 23, 2022 at 10:51 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Fri, Sep 23, 2022 at 10:25:05AM -0700, Daniel Verkamp wrote:
> > Yes, we hit this in crosvm when booting the guest kernel with either
> > OVMF or u-boot on an Intel 12th Gen CPU. The guest kernel boots fine
> > when loaded directly (using the crosvm kernel loader and not running
> > any firmware setup in the guest), but it crashes when booting with
> > firmware inside the first forward memmove() after alternatives are set
> > up (which happens to be in printk). I haven't gotten to the bottom of
> > why exactly using firmware is causing this to be set up in an
> > inconsistent way, but this is a real-world situation, not just a
> > hypothetical.
>
> Sounds like broken virt firmware or so. And if that is not an issue on
> baremetal, then the virt stack should be fixed - not the kernel.
>
> > Now that I look at it with fresh eyes again, maybe we should instead
> > directly patch the memmove FSRM alternative so that the flag-set
> > version just does the same jmp as the ERMS one. I can prepare a patch
> > for that instead of (or in addition to) this one if that sounds
> > better.
>
> So, if the virt firmware deviates from how the real hardware behaves,
> then the kernel needs no fixing.
>
> So you'd have to figure out why is the virt firmware causing this and
> not baremetal.
>
> Then we can talk about fixes.

Hi Borislav,

We found that the IA32_MISC_ENABLE MSR setup was missing in the crosvm
firmware boot path (but not when directly booting a kernel, which is
why it did not get noticed for a while). Setting the fast string bit
in the MSR avoids the issue.

However, I still think it would be appropriate to apply this patch or
something like it, since there could be a CPU, microcode update, BIOS,
etc. that clears this bit while still having the CPUID flags for FSRM
and ERMS. The Intel SDM says: "Software can disable fast-string
operation by clearing the fast-string-enable bit (bit 0) of
IA32_MISC_ENABLE MSR", so it's not an invalid configuration for this
bit to be unset.

Additionally, something like this avoids the problem by making the
FSRM case jump directly to the REP MOVSB rather than falling through
to the ERMS jump in the next instruction, which seems like basically
free insurance (but if the FSRM flag gets used somewhere else in the
future, having it set consistently with ERMS is probably still a good
idea, per the original patch):

diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S
index 724bbf83eb5b..8ac557409c7d 100644
--- a/arch/x86/lib/memmove_64.S
+++ b/arch/x86/lib/memmove_64.S
@@ -38,7 +38,7 @@ SYM_FUNC_START(__memmove)

         /* FSRM implies ERMS => no length checks, do the copy directly */
 .Lmemmove_begin_forward:
-        ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
+        ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "jmp .Lmemmove_erms",
X86_FEATURE_FSRM
         ALTERNATIVE "", "jmp .Lmemmove_erms", X86_FEATURE_ERMS

And hey, this means one less instruction to execute in the FSRM path. :)

Thanks,
-- Daniel