x86, possible bug in __memmove() alternatives patching

x86, possible bug in __memmove() alternatives patching

[Matthias Welwarsky]

Hi list,

I've come across an odd behavior that I'm not sure whether it counts as a bug.
I'm working on an x86 hypervisor at the moment and there was an odd problem 
with a linux guest system dying during booting, but only on an intel NUC11 
(i3-1115G4). It booted normally on any other system I tested on.

The guest was usin kernel version 5.10.89.

Now, the reason eventually turned out to be __memmove() getting broken when 
applying the alternatives. The CPU on the NUC11 has the FSRM feature, which 
enables a particular alternative in __memmove(). However, the code assumes 
that the ERMS feature is always present with FSRM, otherwise the function gets 
broken. Of course this is normally the case, a CPU without ERMS would never 
advertise FSRM.

However: in the function early_intel_setup(), the ERMS feature might get 
cleared depending on the IA32_MISC_ENABLE MSR: if fast string operations are 
disabled through this MSR, X86_FEATURE_ERMS is removed from the available set, 
but X86_FEATURE_FSRM is not removed.

Now, the hypervisor I'm working on emulates this MSR and in the particular 
case, fast string ops were reported as disabled, leading to the above 
mentioned inconsistency in __memmove().

The Intel SDM doesn't go into any details if disabling fast string ops through 
IA32_MISC_ENABLE will also clear FSRM from cpuid leaf 7, but I think that's 
not relevant for the linux kernel anyway because cpuid is parsed before MSRs 
are evaluated.

I know it's is probably a very rare case and Intel recommends having fast 
string ops enabled, hence the question: would this be considered a bug in the 
kernel that should be fixed? A potential fix could be to clear FSRM together 
with ERMS depending on IA32_MISC_ENABLE.


Mit freundlichen Grüßen/Best regards,

Matthias Welwarsky
Project Engineer

SYSGO GmbH
Office Mainz
Am Pfaffenstein 8 / D-55270 Klein-Winternheim / Germany
Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
E-mail: matthias.welwarsky@sysgo.com
_________________________________________________________________________________
Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________
Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884 
Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny (COO) 
USt-Id-Nr./VAT-Id-No.: DE 149062328 

The protection of your personal data is important to us. Under the following 
link 
you can see the information in accordance with article 13 GDPR: 
https://www.sysgo.com/privacy_policy

Re: x86, possible bug in __memmove() alternatives patching

[Borislav Petkov]

+ dhansen for comment on...

On Fri, Mar 25, 2022 at 09:51:09AM +0100, Matthias Welwarsky wrote:
> Hi list,
> 
> I've come across an odd behavior that I'm not sure whether it counts as a bug.
> I'm working on an x86 hypervisor at the moment and there was an odd problem 
> with a linux guest system dying during booting, but only on an intel NUC11 
> (i3-1115G4). It booted normally on any other system I tested on.
> 
> The guest was usin kernel version 5.10.89.
> 
> Now, the reason eventually turned out to be __memmove() getting broken when 
> applying the alternatives. The CPU on the NUC11 has the FSRM feature, which 
> enables a particular alternative in __memmove(). However, the code assumes 
> that the ERMS feature is always present with FSRM, otherwise the function gets 
> broken. Of course this is normally the case, a CPU without ERMS would never 
> advertise FSRM.
> 
> However: in the function early_intel_setup(), the ERMS feature might get 
> cleared depending on the IA32_MISC_ENABLE MSR: if fast string operations are 
> disabled through this MSR, X86_FEATURE_ERMS is removed from the available set, 
> but X86_FEATURE_FSRM is not removed.

... this thing, whether that's really the case on real hw.

Leaving in the rest for reference.

> Now, the hypervisor I'm working on emulates this MSR and in the particular 
> case, fast string ops were reported as disabled, leading to the above 
> mentioned inconsistency in __memmove().
> 
> The Intel SDM doesn't go into any details if disabling fast string ops through 
> IA32_MISC_ENABLE will also clear FSRM from cpuid leaf 7, but I think that's 
> not relevant for the linux kernel anyway because cpuid is parsed before MSRs 
> are evaluated.
> 
> I know it's is probably a very rare case and Intel recommends having fast 
> string ops enabled, hence the question: would this be considered a bug in the 
> kernel that should be fixed? A potential fix could be to clear FSRM together 
> with ERMS depending on IA32_MISC_ENABLE.
> 
> 
> Mit freundlichen Grüßen/Best regards,
> 
> Matthias Welwarsky
> Project Engineer
> 
> SYSGO GmbH
> Office Mainz
> Am Pfaffenstein 8 / D-55270 Klein-Winternheim / Germany
> Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
> E-mail: matthias.welwarsky@sysgo.com
> _________________________________________________________________________________
> Web: https://www.sysgo.com
> Blog: https://www.sysgo.com/blog
> Events: https://www.sysgo.com/events
> Newsletter: https://www.sysgo.com/newsletter
> _________________________________________________________________________________
> Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884 
> Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny (COO) 
> USt-Id-Nr./VAT-Id-No.: DE 149062328 
> 
> The protection of your personal data is important to us. Under the following 
> link 
> you can see the information in accordance with article 13 GDPR: 
> https://www.sysgo.com/privacy_policy
> 
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: x86, possible bug in __memmove() alternatives patching

[Dave Hansen]

On 3/25/22 15:07, Borislav Petkov wrote:
> I know it's is probably a very rare case and Intel recommends having fast 
> string ops enabled, hence the question: would this be considered a bug in the 
> kernel that should be fixed? A potential fix could be to clear FSRM together 
> with ERMS depending on IA32_MISC_ENABLE.

I'd consider it a bug in the hypervisor, personally. ;)

But, we do try to make the kernel work even the face of funky
hypervisors that do things that never occur on real hardware.  If a nice
patch to fix this up showed up, I'd definitely take a look.

Re: x86, possible bug in __memmove() alternatives patching

[Borislav Petkov]

On Fri, Mar 25, 2022 at 09:45:24PM -0700, Dave Hansen wrote:
> On 3/25/22 15:07, Borislav Petkov wrote:
> > I know it's is probably a very rare case and Intel recommends having fast 
> > string ops enabled, hence the question: would this be considered a bug in the 
> > kernel that should be fixed? A potential fix could be to clear FSRM together 
> > with ERMS depending on IA32_MISC_ENABLE.
> 
> I'd consider it a bug in the hypervisor, personally. ;)

That's a given.

> 
> But, we do try to make the kernel work even the face of funky
> hypervisors that do things that never occur on real hardware.  If a nice
> patch to fix this up showed up, I'd definitely take a look.

So, more to the point, it is about this chunk:

        /*
         * If fast string is not enabled in IA32_MISC_ENABLE for any reason,
         * clear the fast string and enhanced fast string CPU capabilities.
         */
        if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
                rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
                if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
                        pr_info("Disabled fast string operations\n");
                        setup_clear_cpu_cap(X86_FEATURE_REP_GOOD);
                        setup_clear_cpu_cap(X86_FEATURE_ERMS);
                }
        }

we already check whether fast strings was disabled, regardless of HV or
not. Question is, should we clear X86_FEATURE_FSRM there too. I wanna
say yes.

Or is it that, *if* MSR_IA32_MISC_ENABLE_FAST_STRING is clear, the FSRM
CPUID bit was not set either so nothing to clear...

Hmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: x86, possible bug in __memmove() alternatives patching

[Matthias Welwarsky]

On Samstag, 26. März 2022 05:45:24 CET Dave Hansen wrote:
> On 3/25/22 15:07, Borislav Petkov wrote:
> > I know it's is probably a very rare case and Intel recommends having fast
> > string ops enabled, hence the question: would this be considered a bug in
> > the kernel that should be fixed? A potential fix could be to clear FSRM
> > together with ERMS depending on IA32_MISC_ENABLE.
> 
> I'd consider it a bug in the hypervisor, personally. ;)

Of course, no doubt about that.

> But, we do try to make the kernel work even the face of funky
> hypervisors that do things that never occur on real hardware.  If a nice
> patch to fix this up showed up, I'd definitely take a look.

The question is whether a sequence like this could be relevant:

0) CPU announces feature FSRM through cpuid
1) BIOS/firmware disables fast string ops through IA32_MISC_ENABLE before 
loading kernel (for whatever reason)
2) Kernel populates features from cpuid
3) Kernel clears ERMS based on IA32_MISC_ENABLE
4) "alternatives" patching destroys __memmove()

That depends on whether a cpuid instruction after step 1) would still announce 
FSRM. If not, then the whole point is moot and no patch needed (unless we want 
to guard against hypervisor bugs).

-- 
Mit freundlichen Grüßen/Best regards,

Matthias Welwarsky
Project Engineer

SYSGO GmbH
Office Mainz
Am Pfaffenstein 8 / D-55270 Klein-Winternheim / Germany
Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
E-mail: matthias.welwarsky@sysgo.com
_________________________________________________________________________________
Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________
Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884 
Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny (COO) 
USt-Id-Nr./VAT-Id-No.: DE 149062328 

The protection of your personal data is important to us. Under the following 
link 
you can see the information in accordance with article 13 GDPR: 
https://www.sysgo.com/privacy_policy

Re: x86, possible bug in __memmove() alternatives patching

[Dave Hansen]

On 3/26/22 04:39, Matthias Welwarsky wrote:
> 
>> But, we do try to make the kernel work even the face of funky
>> hypervisors that do things that never occur on real hardware.  If a nice
>> patch to fix this up showed up, I'd definitely take a look.
> The question is whether a sequence like this could be relevant:
> 
> 0) CPU announces feature FSRM through cpuid
> 1) BIOS/firmware disables fast string ops through IA32_MISC_ENABLE before 
> loading kernel (for whatever reason)
> 2) Kernel populates features from cpuid
> 3) Kernel clears ERMS based on IA32_MISC_ENABLE
> 4) "alternatives" patching destroys __memmove()

Hi Matthias,

What does "destroys __memmove()" mean in practice?  What's the end-user
visible effect of this?  Do they see a crash or just crummy performance?

Re: x86, possible bug in __memmove() alternatives patching

[Dave Hansen]

On 3/26/22 01:27, Borislav Petkov wrote:
> So, more to the point, it is about this chunk:
> 
>         /*
>          * If fast string is not enabled in IA32_MISC_ENABLE for any reason,
>          * clear the fast string and enhanced fast string CPU capabilities.
>          */
>         if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
>                 rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>                 if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
>                         pr_info("Disabled fast string operations\n");
>                         setup_clear_cpu_cap(X86_FEATURE_REP_GOOD);
>                         setup_clear_cpu_cap(X86_FEATURE_ERMS);
>                 }
>         }
> 
> we already check whether fast strings was disabled, regardless of HV or
> not. Question is, should we clear X86_FEATURE_FSRM there too. I wanna
> say yes.

I don't think it would hurt to clear it.

> Or is it that, *if* MSR_IA32_MISC_ENABLE_FAST_STRING is clear, the FSRM
> CPUID bit was not set either so nothing to clear...

I don't think there's really any direct connection between the CPUID bit
and MSR_IA32_MISC_ENABLE_FAST_STRING.  The CPUID bit definitely doesn't
appear to be cleared by the CPU if MSR_IA32_MISC_ENABLE_FAST_STRING is
clear.

Re: x86, possible bug in __memmove() alternatives patching

[Matthias Welwarsky]

On Mittwoch, 30. März 2022 00:33:17 CEST Dave Hansen wrote:
> On 3/26/22 04:39, Matthias Welwarsky wrote:
> >> But, we do try to make the kernel work even the face of funky
> >> hypervisors that do things that never occur on real hardware.  If a nice
> >> patch to fix this up showed up, I'd definitely take a look.
> > 
> > The question is whether a sequence like this could be relevant:
> > 
> > 0) CPU announces feature FSRM through cpuid
> > 1) BIOS/firmware disables fast string ops through IA32_MISC_ENABLE before
> > loading kernel (for whatever reason)
> > 2) Kernel populates features from cpuid
> > 3) Kernel clears ERMS based on IA32_MISC_ENABLE
> > 4) "alternatives" patching destroys __memmove()
> 
> Hi Matthias,
> 
> What does "destroys __memmove()" mean in practice?  What's the end-user
> visible effect of this?  Do they see a crash or just crummy performance?

Solid kernel freeze in my case. No Oops, boot just hangs right after 
__memmove() was patched. Not easily trackable.

I'll send a patch. I think the same rationale applies to FSRM as to ERMS, 
which gets manually cleared when IA32_MISC_ENABLE says that fast string ops 
are not available. It will be a one liner added to the dependency table in 
cpu-deps.c, making FSRM depend on ERMS so that it gets automatically cleared.

The way __memmove gets broken is kind of obvious if you look at the code.

Here's the relevant bits:

        /* FSRM implies ERMS => no length checks, do the copy directly */
.Lmemmove_begin_forward:
        ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
        ALTERNATIVE "", __stringify(movq %rdx, %rcx; rep movsb; RET), 
X86_FEATURE_ERMS

If FSRM is there but ERMS isn't, the first ALTERNATIVE is activated but not 
the second one. That means the length check (< 32) and subsequent "jb 1f" is 
suppressed but the "movq %rdx, %rcx; rep movsb; RET" is also not there. 

So, if the amount to be moved is < 32, it executes a code path that relies on 
having at least 32 byte to copy and that results in doing all kinds of stuff 
but not what you'd expect. I haven't analyzed what happens in depth, but it 
might move more data than requested, or nothing at all.


-- 
Mit freundlichen Grüßen/Best regards,

Matthias Welwarsky
Project Engineer

SYSGO GmbH
Office Mainz
Am Pfaffenstein 8 / D-55270 Klein-Winternheim / Germany
Phone: +49-6136-9948-0 / Fax: +49-6136-9948-10
E-mail: matthias.welwarsky@sysgo.com
_________________________________________________________________________________
Web: https://www.sysgo.com
Blog: https://www.sysgo.com/blog
Events: https://www.sysgo.com/events
Newsletter: https://www.sysgo.com/newsletter
_________________________________________________________________________________
Handelsregister/Commercial Registry: HRB Mainz 90 HRB 48884 
Geschäftsführung/Managing Directors: Etienne Butery (CEO), Kai Sablotny (COO) 
USt-Id-Nr./VAT-Id-No.: DE 149062328 

The protection of your personal data is important to us. Under the following 
link 
you can see the information in accordance with article 13 GDPR: 
https://www.sysgo.com/privacy_policy

Re: x86, possible bug in __memmove() alternatives patching

[Dave Hansen]

On 3/30/22 06:56, Matthias Welwarsky wrote:
> 
> Here's the relevant bits:
> 
>         /* FSRM implies ERMS => no length checks, do the copy directly */
> .Lmemmove_begin_forward:
>         ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
>         ALTERNATIVE "", __stringify(movq %rdx, %rcx; rep movsb; RET), 
> X86_FEATURE_ERMS
> 
> If FSRM is there but ERMS isn't, the first ALTERNATIVE is activated but not 
> the second one. That means the length check (< 32) and subsequent "jb 1f" is 
> suppressed but the "movq %rdx, %rcx; rep movsb; RET" is also not there. 

Ahh, thanks for the explanation.  It would help if I wasn't reading the
code wrong.

> I'll send a patch. I think the same rationale applies to FSRM as to ERMS, 
> which gets manually cleared when IA32_MISC_ENABLE says that fast string ops 
> are not available. It will be a one liner added to the dependency table in 
> cpu-deps.c, making FSRM depend on ERMS so that it gets automatically cleared.

Sounds good.  Could you also add some of that explanation to a comment
__memmove and basically say that the code is broken if the dependency
isn't enforced?

Re: x86, possible bug in __memmove() alternatives patching

[Borislav Petkov]

On Wed, Mar 30, 2022 at 03:56:52PM +0200, Matthias Welwarsky wrote:
> Here's the relevant bits:
> 
>         /* FSRM implies ERMS => no length checks, do the copy directly */
> .Lmemmove_begin_forward:
>         ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
>         ALTERNATIVE "", __stringify(movq %rdx, %rcx; rep movsb; RET), 
> X86_FEATURE_ERMS
> 
> If FSRM is there but ERMS isn't, the first ALTERNATIVE is activated but not 
> the second one. That means the length check (< 32) and subsequent "jb 1f" is 
> suppressed but the "movq %rdx, %rcx; rep movsb; RET" is also not there.

Yap, this is what the live, patched code looks like below.

Basically, both alternatives are NOPped out so execution wanders off
somewhere into the weeds.

The kernel has exploded somewhere later during init:

#0  delay_tsc (cycles=2976021) at arch/x86/lib/delay.c:79
#1  0xffffffff81954bd3 in panic (fmt=fmt@entry=0xffffffff82108368 "Attempted to kill init! exitcode=0x%08x\n")
    at kernel/panic.c:359
#2  0xffffffff81954f67 in do_exit (code=<optimized out>, code@entry=9) at kernel/exit.c:775
#3  0xffffffff8108d921 in make_task_dead (signr=9) at kernel/exit.c:898
#4  0xffffffff81001cf7 in rewind_stack_and_make_dead () at arch/x86/entry/entry_64.S:1439
#5  0x0000000000000000 in ?? ()

with the last thing on the console saying:

[    0.200955] Freeing SMP alternatives memory: 32K
<EOF>


(gdb) disas/rs __memmove,+40
Dump of assembler code from 0xffffffff815079a0 to 0xffffffff815079c8:
arch/x86/lib/memmove_64.S:
29              mov %rdi, %rax
   0xffffffff815079a0 <memmove+0>:      48 89 f8        mov    %rdi,%rax

30
31              /* Decide forward/backward copy mode */
32              cmp %rdi, %rsi
   0xffffffff815079a3 <memmove+3>:      48 39 fe        cmp    %rdi,%rsi

33              jge .Lmemmove_begin_forward
   0xffffffff815079a6 <memmove+6>:      7d 0f   jge    0xffffffff815079b7 <__memmove+23>

34              mov %rsi, %r8
   0xffffffff815079a8 <__memmove+8>:    49 89 f0        mov    %rsi,%r8

35              add %rdx, %r8
   0xffffffff815079ab <__memmove+11>:   49 01 d0        add    %rdx,%r8

36              cmp %rdi, %r8
   0xffffffff815079ae <__memmove+14>:   49 39 f8        cmp    %rdi,%r8

37              jg 2f
   0xffffffff815079b1 <__memmove+17>:   0f 8f a9 00 00 00       jg     0xffffffff81507a60 <__memmove+192>

38
39              /* FSRM implies ERMS => no length checks, do the copy directly */
40      .Lmemmove_begin_forward:
41              ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
   0xffffffff815079b7 <__memmove+23>:   0f 1f 84 00 00 00 00 00 nopl   0x0(%rax,%rax,1)
   0xffffffff815079bf <__memmove+31>:   66 90   xchg   %ax,%ax
   0xffffffff815079c1 <__memmove+33>:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)

42              ALTERNATIVE "", __stringify(movq %rdx, %rcx; rep movsb; RET), X86_FEATURE_ERMS
43
44              /*
45               * movsq instruction have many startup latency
46               * so we handle small size by general register.
47               */
48              cmp  $680, %rdx
   0xffffffff815079c7 <__memmove+39>:   48 81 fa a8 02 00 00    cmp    $0x2a8,%rdx

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette