1468277 - MIPS spectre mitigation

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[qiaopengcheng]

User Agent: Mozilla/5.0 (X11; Linux mips64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180523145036

Steps to reproduce:

on mips platform, about at least 2490 cases of jit-test are failed for spectre migation flags.

cd jit-test
./jit-test --tbpl ../release_OPT.OBJ/dist/bin/js


Actual results:

[3785|2491| 0| 0] 100% ==========================================>| 192.6s

failed 2491


Expected results:

mips platform should supporting these testing cases.

Comment 1

[qiaopengcheng]

Attachment: Implementation the Spectre functions for MIPS

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8984924 [details] [diff] [review]
Implementation the Spectre functions for MIPS

Review of attachment 8984924 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler-inl.h
@@ +459,5 @@
>      loadPtr(Address(obj, JSObject::offsetOfGroup()), scratch);
>      branchPtr(cond, Address(scratch, ObjectGroup::offsetOfClasp()), ImmPtr(clasp), label);
>  
> +    if (JitOptions.spectreObjectMitigationsMisc) {
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)

Hm I don't like the #ifdefs in all this code..

Is Spectre really a concern for MIPS CPUs? I think it might be better to just always disable our mitigations on MIPS.

Comment 3

[qiaopengcheng]

Very sorry, reply so late.


> Is Spectre really a concern for MIPS CPUs? I think it might be better to
> just always disable our mitigations on MIPS.


Yes, spectre does not only affect the intel and arm processors.

About mips information,
https://www.mips.com/blog/mips-response-on-speculative-execution-and-side-channel-vulnerabilities/
https://www.mips.com/forums/topic/mips-mitigations-for-side-channel-vulnerabilities-on-speculative-execution-cpus/

Comment 4

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8984924 [details] [diff] [review]
> Implementation the Spectre functions for MIPS
> 
> Review of attachment 8984924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MacroAssembler-inl.h
> @@ +459,5 @@
> >      loadPtr(Address(obj, JSObject::offsetOfGroup()), scratch);
> >      branchPtr(cond, Address(scratch, ObjectGroup::offsetOfClasp()), ImmPtr(clasp), label);
> >  
> > +    if (JitOptions.spectreObjectMitigationsMisc) {
> > +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> 
> Hm I don't like the #ifdefs in all this code..
> 
> Is Spectre really a concern for MIPS CPUs? I think it might be better to
> just always disable our mitigations on MIPS.




The latest inbound can't start with a segment-fault on mips64-linux.

./mach configure; ./mach build

gdb ./objdir/dist/bin/firefox

[New Thread 0xffee6a31d0 (LWP 29353)]
[Thread 0xffee6a31d0 (LWP 29353) exited]
Detaching after fork from child process 29354.
[New Thread 0xffee6a31d0 (LWP 29355)]
[New Thread 0xffa75ff1d0 (LWP 29358)]
[New Thread 0xffa6dff1d0 (LWP 29359)]
[New Thread 0xffa65ff1d0 (LWP 29360)]
[New Thread 0xffa5dff1d0 (LWP 29361)]
[New Thread 0xffa55ff1d0 (LWP 29362)]
[New Thread 0xffa53ff1d0 (LWP 29363)]
[New Thread 0xffa51ff1d0 (LWP 29364)]
[New Thread 0xffa4fff1d0 (LWP 29365)]
[New Thread 0xffa47ff1d0 (LWP 29366)]
[New Thread 0xffa45ff1d0 (LWP 29367)]
[New Thread 0xffa42ff1d0 (LWP 29368)]
[New Thread 0xffa3aff1d0 (LWP 29369)]
[New Thread 0xffa32ff1d0 (LWP 29370)]
[New Thread 0xffa27a71d0 (LWP 29372)]
[New Thread 0xffa1cff1d0 (LWP 29374)]
Detaching after fork from child process 29375.

Program received signal SIGSEGV, Segmentation fault.
js::jit::MacroAssembler::spectreMovePtr (cond=js::jit::AssemblerMIPSShared::NotEqual, this=0xffffff5dc0, src=..., dest=...)
    at /mnt/home/qiao/work_qiao/open_src/gecko-dev/js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h:1013
1013        MOZ_CRASH();




Even disable spectre as following, the inbound-firefox also crashed with a segment-fault.

--- a/js/src/jit/JitOptions.cpp
+++ b/js/src/jit/JitOptions.cpp
@@ -227,12 +227,12 @@ DefaultJitOptions::DefaultJitOptions()
             Warn(forcedRegisterAllocatorEnv, env);
     }
 
-    SET_DEFAULT(spectreIndexMasking, true);
-    SET_DEFAULT(spectreObjectMitigationsBarriers, true);
-    SET_DEFAULT(spectreObjectMitigationsMisc, true);
-    SET_DEFAULT(spectreStringMitigations, true);
-    SET_DEFAULT(spectreValueMasking, true);
-    SET_DEFAULT(spectreJitToCxxCalls, true);
+    SET_DEFAULT(spectreIndexMasking, false);
+    SET_DEFAULT(spectreObjectMitigationsBarriers, false);
+    SET_DEFAULT(spectreObjectMitigationsMisc, false);
+    SET_DEFAULT(spectreStringMitigations, false);
+    SET_DEFAULT(spectreValueMasking, false);
+    SET_DEFAULT(spectreJitToCxxCalls, false);



--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -8575,12 +8575,13 @@ SetContextOptions(JSContext* cx, const OptionParser& op)
 
     if (const char* str = op.getStringOption("spectre-mitigations")) {
         if (strcmp(str, "on") == 0) {
-            jit::JitOptions.spectreIndexMasking = true;
-            jit::JitOptions.spectreObjectMitigationsBarriers = true;
-            jit::JitOptions.spectreObjectMitigationsMisc = true;
-            jit::JitOptions.spectreStringMitigations = true;
-            jit::JitOptions.spectreValueMasking = true;
-            jit::JitOptions.spectreJitToCxxCalls = true;
+            //jit::JitOptions.spectreIndexMasking = true;
+            //jit::JitOptions.spectreObjectMitigationsBarriers = true;
+            //jit::JitOptions.spectreObjectMitigationsMisc = true;
+            //jit::JitOptions.spectreStringMitigations = true;
+            //jit::JitOptions.spectreValueMasking = true;
+            //jit::JitOptions.spectreJitToCxxCalls = true;
+            std::printf("---------on spectre-mitigations! \n");
         } else if (strcmp(str, "off") == 0) {
             jit::JitOptions.spectreIndexMasking = false;
             jit::JitOptions.spectreObjectMitigationsBarriers = false;




But if adding the spectre-mips patch, can start normally.

So please considering how to merge this patch on mips-platform.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to qiaopengcheng from comment #4)
> Even disable spectre as following, the inbound-firefox also crashed with a
> segment-fault.

I think you can fix that by just implementing the cmp32* and test32* functions you have in the patch, right?

It would be good to make sure we don't have a bug in the Spectre prefs.

Comment 6

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to qiaopengcheng from comment #4)
> > Even disable spectre as following, the inbound-firefox also crashed with a
> > segment-fault.
> 
> I think you can fix that by just implementing the cmp32* and test32*
> functions you have in the patch, right?
> 
> It would be good to make sure we don't have a bug in the Spectre prefs.


The modified files in the patch are these:
     jit/MacroAssembler-inl.h
     jit/MacroAssembler.cpp
     jit/mips-shared/MacroAssembler-mips-shared-inl.h
     jit/mips-shared/MacroAssembler-mips-shared.cpp
     jit/mips-shared/MacroAssembler-mips-shared.h


What you mean is that don't modify the arch-independed files of jit/MacroAssembler.cpp/h ?
Just only modify the files under directory of jit/mips-share/.


Because the mips-ISA does not have the flag registor whick likes ARM or X64 updating the processor stating flag
and can get the last comparing result by accessing the processor state flag.

On mips-arch platform, these functions like cmp32* and test32*, must need the compared registors where saving the comparing data. But the function definition doesn't pass these registors.

Before entering these functions during jit/MacroAssembler.cpp/h  saving the comparing registors to vales of Register spectreCmpReg1/spectreCmpReg2 defined within the file jit/mips-shared/MacroAssembler-mips-shared.h.

Comment 7

[Jan de Mooij [:jandem]]

So if you (1) Fix cmp32* and test32* etc on MIPS *without the arch-independent* changes and (2) disable Spectre flags, does that fix the Firefox crash on MIPS?

Comment 8

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #7)
> So if you (1) Fix cmp32* and test32* etc on MIPS *without the
> arch-independent* changes and (2) disable Spectre flags, does that fix the
> Firefox crash on MIPS?

It can't work.
js crashes for error.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to qiaopengcheng from comment #8)
> It can't work.
> js crashes for error.

Why? spectreMovePtr for example is only called if Spectre mitigations are *enabled*.

My question is: do you need the changes to the spectre* functions if mitigations are disabled. And if so, why?

Comment 10

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #9)
> (In reply to qiaopengcheng from comment #8)
> > It can't work.
> > js crashes for error.
> 
> Why? spectreMovePtr for example is only called if Spectre mitigations are
> *enabled*.
> 
> My question is: do you need the changes to the spectre* functions if
> mitigations are disabled. And if so, why?


Program received signal SIGSEGV, Segmentation fault.
js::jit::MacroAssembler::spectreMovePtr (cond=js::jit::AssemblerMIPSShared::NotEqual, this=0xffffff5dc0, src=..., dest=...)
    at /mnt/home/qiao/work_qiao/open_src/gecko-dev/js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h:1013
1013        MOZ_CRASH();


Even disable spectre as following, the inbound-firefox also crashed with a segment-fault.

--- a/js/src/jit/JitOptions.cpp
+++ b/js/src/jit/JitOptions.cpp
@@ -227,12 +227,12 @@ DefaultJitOptions::DefaultJitOptions()
             Warn(forcedRegisterAllocatorEnv, env);
     }
 
-    SET_DEFAULT(spectreIndexMasking, true);
-    SET_DEFAULT(spectreObjectMitigationsBarriers, true);
-    SET_DEFAULT(spectreObjectMitigationsMisc, true);
-    SET_DEFAULT(spectreStringMitigations, true);
-    SET_DEFAULT(spectreValueMasking, true);
-    SET_DEFAULT(spectreJitToCxxCalls, true);
+    SET_DEFAULT(spectreIndexMasking, false);
+    SET_DEFAULT(spectreObjectMitigationsBarriers, false);
+    SET_DEFAULT(spectreObjectMitigationsMisc, false);
+    SET_DEFAULT(spectreStringMitigations, false);
+    SET_DEFAULT(spectreValueMasking, false);
+    SET_DEFAULT(spectreJitToCxxCalls, false);


--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -8575,12 +8575,13 @@ SetContextOptions(JSContext* cx, const OptionParser& op)
 
     if (const char* str = op.getStringOption("spectre-mitigations")) {
         if (strcmp(str, "on") == 0) {
-            jit::JitOptions.spectreIndexMasking = true;
-            jit::JitOptions.spectreObjectMitigationsBarriers = true;
-            jit::JitOptions.spectreObjectMitigationsMisc = true;
-            jit::JitOptions.spectreStringMitigations = true;
-            jit::JitOptions.spectreValueMasking = true;
-            jit::JitOptions.spectreJitToCxxCalls = true;
+            //jit::JitOptions.spectreIndexMasking = true;
+            //jit::JitOptions.spectreObjectMitigationsBarriers = true;
+            //jit::JitOptions.spectreObjectMitigationsMisc = true;
+            //jit::JitOptions.spectreStringMitigations = true;
+            //jit::JitOptions.spectreValueMasking = true;
+            //jit::JitOptions.spectreJitToCxxCalls = true;
+            std::printf("---------on spectre-mitigations! \n");
         } else if (strcmp(str, "off") == 0) {
             jit::JitOptions.spectreIndexMasking = false;
             jit::JitOptions.spectreObjectMitigationsBarriers = false;


Now the fact is that firefox-inbound will crash for spectre functions and no matter the disable or default enable.
I don't know why firefox starting will go here.

But if you only test the jit-test cases, the disable spectre will work.

Comment 11

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #9)
> (In reply to qiaopengcheng from comment #8)
> > It can't work.
> > js crashes for error.
> 
> Why? spectreMovePtr for example is only called if Spectre mitigations are
> *enabled*.
> 
> My question is: do you need the changes to the spectre* functions if
> mitigations are disabled. And if so, why?



Like comment 3  web-linking-address, spectre functions are needed for mips-processors as the cpu-core  micro-design is used the related speculative-execution.

Comment 12

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #9)
> (In reply to qiaopengcheng from comment #8)
> > It can't work.
> > js crashes for error.
> 
> Why? spectreMovePtr for example is only called if Spectre mitigations are
> *enabled*.
> 
> My question is: do you need the changes to the spectre* functions if
> mitigations are disabled. And if so, why?



If adding this patch, Disable the spectre mitigations that also works and adding it can make it working on mips by default.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8984924 [details] [diff] [review]
Implementation the Spectre functions for MIPS

I'm sorry, parts of this patch are fine but the spectreCmpReg1 and spectreCmpReg2 use is too much of a hack IMO.

I still think enabling Spectre mitigations on MIPS is probably overkill, but I could be convinced if there's a nicer way to achieve it.

Dragan, any thoughts?

Comment 14

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #13)
> Comment on attachment 8984924 [details] [diff] [review]
> Implementation the Spectre functions for MIPS
> 
> I'm sorry, parts of this patch are fine but the spectreCmpReg1 and
> spectreCmpReg2 use is too much of a hack IMO.
> 
> I still think enabling Spectre mitigations on MIPS is probably overkill, but
> I could be convinced if there's a nicer way to achieve it.
> 
> Dragan, any thoughts?


As from comment 6, there is no flag registor whick likes ARM or X64 updating the processor stating flag.
And then can get the last comparing result by accessing the processor state flag.


For example in file js/src/jit/MacroAssembler.cpp when working on ARM64,

 264             if (JitOptions.spectreObjectMitigationsBarriers) {
 265                 if (--numBranches > 0) {
 266                     Label next;
 267                     branchPtr(NotEqual, obj, ImmGCPtr(singleton), &next); //During this function arm will update and save the comparing result to the system processor stating flag registor.
 268 #if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
 269                     spectreCmpReg1 = obj;
 270                     spectreCmpReg2 = ScratchRegister;
 271 #endif
 272                     spectreMovePtr(NotEqual, scratch, spectreRegToZero); //Here can access the system processor stating flag registor for getting the previous comparing results.
 273                     jump(&matched);
 274                     bind(&next);
 275                 } else {
 276                     branchPtr(NotEqual, obj, ImmGCPtr(singleton), miss);
 277 #if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
 278                     spectreCmpReg1 = obj;
 279                     spectreCmpReg2 = ScratchRegister;
 280 #endif
 281                     spectreMovePtr(NotEqual, scratch, spectreRegToZero);
 282                 }
 283             } else {



In line-267 above "    branchPtr(NotEqual, obj, ImmGCPtr(singleton), &next); //During this function arm will update and save the comparing result to the system processor stating flag registor."



the function "branchPtr" on arm64 will finally call a like funtion  in file js/src/jit/arm64/MacroAssembler-arm64-inl.h,

1065 MacroAssembler::branchPtr(Condition cond, Register lhs, Register rhs, L label)
1066 {   
1067     Cmp(ARMRegister(lhs, 64), ARMRegister(rhs, 64));   //Here will update and save the comparing result to system processor stating flag registor
1068     B(label, cond);
1069 }

So, on arm and x86/64 processors, there is no need to saving the Cmp registors.


But unfortunately there is no similar registor for mips-processors.

So for mips-arch processors, the code in file js/src/jit/MacroAssembler.cpp
 272                     spectreMovePtr(NotEqual, scratch, spectreRegToZero); 
//arm and intel processors here can access the system processor stating flag registor for getting the previous comparing results.
But for mips processors, it must have to know the original comparing-registors, where the function "spectreMovePtr" does't passing these argments.
that's why using the spectreCmpReg1/spectreCmpReg2 values  on MIPS.

Comment 15

[Jan de Mooij [:jandem]]

Yeah, I understand the problem with the flags register on MIPS, I'm just looking for a nicer design :)

Maybe we could add spectreCmpReg1/spectreCmpReg2 as extra arguments to these spectre* functions and only use the arguments on MIPS? Other platforms could just ignore them.

Comment 16

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #15)
> Yeah, I understand the problem with the flags register on MIPS, I'm just
> looking for a nicer design :)
> 
> Maybe we could add spectreCmpReg1/spectreCmpReg2 as extra arguments to these
> spectre* functions and only use the arguments on MIPS? Other platforms could
> just ignore them.

Thanks， I get it.
*^_^*

Comment 17

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #15)
> Yeah, I understand the problem with the flags register on MIPS, I'm just
> looking for a nicer design :)
> 
> Maybe we could add spectreCmpReg1/spectreCmpReg2 as extra arguments to these
> spectre* functions and only use the arguments on MIPS? Other platforms could
> just ignore them.


Is it liking this,


--- a/js/src/jit/MacroAssembler.h
+++ b/js/src/jit/MacroAssembler.h
@@ -1341,11 +1341,17 @@ class MacroAssembler : public MacroAssemblerSpecific
 
     // Conditional move for Spectre mitigations.
     inline void spectreMovePtr(Condition cond, Register src, Register dest)
-        DEFINED_ON(arm, arm64, mips_shared, x86, x64);
+        DEFINED_ON(arm, arm64, x86, x64);
+    inline void spectreMovePtr(Condition cond, Register src, Register dest,
+                               Register spectreCmpRegL, Register spectreCmpRegR)
+        DEFINED_ON(mips_shared);
 
     // Zeroes dest if the condition is true.
     inline void spectreZeroRegister(Condition cond, Register scratch, Register dest)
-        DEFINED_ON(arm, arm64, mips_shared, x86_shared);
+        DEFINED_ON(arm, arm64, x86_shared);
+    inline void spectreZeroRegister(Condition cond, Register scratch, Register dest,
+                                    Register spectreCmpRegL, Register spectreCmpRegR)
+        DEFINED_ON(mips_shared);



--- a/js/src/jit/MacroAssembler.cpp
+++ b/js/src/jit/MacroAssembler.cpp
@@ -265,11 +265,19 @@ MacroAssembler::guardObjectType(Register obj, const TypeSet* types, Register scr
                 if (--numBranches > 0) {
                     Label next;
                     branchPtr(NotEqual, obj, ImmGCPtr(singleton), &next);
+#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
+                    spectreMovePtr(NotEqual, scratch, spectreRegToZero, obj, ScratchRegister);
+#else
                     spectreMovePtr(NotEqual, scratch, spectreRegToZero);
+#endif




--- a/js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
+++ b/js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
 void
-MacroAssembler::spectreMovePtr(Condition cond, Register src, Register dest)
+MacroAssembler::spectreMovePtr(Condition cond, Register src, Register dest,
+                               Register spectreCmpRegL, Register spectreCmpRegR)
 {
-    MOZ_CRASH();
+    Label skip;
+    switch (cond) {
+      case NotEqual:
+        ma_b(spectreCmpRegL, spectreCmpRegR, &skip, Assembler::Equal, ShortJump);
+        ma_move(dest, src);
+        break;
+      case Equal:
+        ma_b(spectreCmpRegL, spectreCmpRegR, &skip, Assembler::NotEqual, ShortJump);
+        ma_move(dest, src);
+        break;
+      default:
+        MOZ_CRASH("NYI");
+    }
+    bind(&skip);
 }

Comment 18

[Jan de Mooij [:jandem]]

Hm actually I don't think this works as Spectre mitigation because you still just have a branch. Instead of 1 (Spectre-vulnerable) branch you now have 2 branches so it doesn't really change anything. On other platforms we use a CMOV to avoid the branch predictor and that prevents speculation.

I suggest waiting for Dragan to comment, the "owner" of the MIPS backend.

Comment 19

[Dragan Mladjenovic]

(In reply to Jan de Mooij [:jandem] from comment #18)
> Hm actually I don't think this works as Spectre mitigation because you still
> just have a branch. Instead of 1 (Spectre-vulnerable) branch you now have 2
> branches so it doesn't really change anything. On other platforms we use a
> CMOV to avoid the branch predictor and that prevents speculation.
> 
> I suggest waiting for Dragan to comment, the "owner" of the MIPS backend.

Yes he has to use MOVZ or MOVN for that. Keep in mind that test32LoadPtr and spectreBoundsCheck32 are also not spectre safe (https://bug1442217.bmoattachments.org/attachment.cgi?id=8955102).

Comment 20

[qiaopengcheng]

(In reply to Dragan Mladjenovic from comment #19)
> (In reply to Jan de Mooij [:jandem] from comment #18)
> > Hm actually I don't think this works as Spectre mitigation because you still
> > just have a branch. Instead of 1 (Spectre-vulnerable) branch you now have 2
> > branches so it doesn't really change anything. On other platforms we use a
> > CMOV to avoid the branch predictor and that prevents speculation.
> > 
> > I suggest waiting for Dragan to comment, the "owner" of the MIPS backend.
> 
> Yes he has to use MOVZ or MOVN for that. Keep in mind that test32LoadPtr and
> spectreBoundsCheck32 are also not spectre safe
> (https://bug1442217.bmoattachments.org/attachment.cgi?id=8955102).

Thanks a lot !

Comment 21

[Dragan Mladjenovic]

Comment on attachment 8984924 [details] [diff] [review]
Implementation the Spectre functions for MIPS

Review of attachment 8984924 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler-inl.h
@@ +461,5 @@
>  
> +    if (JitOptions.spectreObjectMitigationsMisc) {
> +#if defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64)
> +        spectreCmpReg1 = SecondScratchReg;
> +        spectreCmpReg2 = ScratchRegister;

The tricky think here is that you can relay on ScratchRegister to be alive across non ShortJump branches. The branch may be out of range and the expansion will clobber ScratchRegister.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared.h
@@ +237,5 @@
> +    // can get the last comparing result by accessing the processor state flag.
> +    // But for MIPS-arch implementation of the Spectre mitigation operations
> +    // has to saving the comparing registors for getting the comparing result.
> +    Register spectreCmpReg1;
> +    Register spectreCmpReg2;

Thease should either be parameters to their using functions or at least be encapsulated by an method call and maybe even by an scoping class; AutoSpecreScratchRegisters or something.

Another solution is to introduce a couple of MacroAssembler::branch* wrappers that will that will ensure that we always have "branch flag" result in SecondScratchReg. It looks that for now only branchPtr is used in conjunction with spectreMovePtr and spectreZeroRegister. Probably we should rename the spectreMovePtr -> to spectreMoveRegister or spectreZeroRegister -> spectreZeroPtr. 


I think of something like branchPtr -> branchPtrForSpectre that will ensure that on Mips SecondSratchReg holds zero value if branch is not taken and that for later methods. This is a bit fragile because it relays that people using those methods pass the same Conditions as they passed to the branch before. Maybe stash the last cond used by  
branchPtrForSpectre and assert that it is the same the one used by spectreZeroRegister in DEBUG builds.

Comment 22

[Dragan Mladjenovic]

(In reply to Jan de Mooij [:jandem] from comment #13)
> Comment on attachment 8984924 [details] [diff] [review]
> Implementation the Spectre functions for MIPS
> 
> I'm sorry, parts of this patch are fine but the spectreCmpReg1 and
> spectreCmpReg2 use is too much of a hack IMO.
> 
> I still think enabling Spectre mitigations on MIPS is probably overkill, but
> I could be convinced if there's a nicer way to achieve it.
> 
> Dragan, any thoughts?

Sorry for not noticing the issue earlier, was bussy.

I run test on mips32 with --args="--spectre-mitigations=off" in order to get the tests passing. I stopped my own patch series when I first stumbled upon the spectreZeroRegister like change. I don't wish to be that guy to disable security for the sake of convenience, so I think it is better to  be enabled by default albeit crashing, so the user can dwell on it. :) I haven got time lately to look into the whole current state of mitigation in Spidermonkey to evaluate which of these approaches is better. One is to simply put the SYNC barrier  instruction into the spectreZeroRegister and call it a day. I admit it is a bit of an sledgehammer, but so is implementing conditional moves with branches (I think I added MOVN, MOVZ into assembler, so those could be used). Other is to combine the preceding branches and register zeroing into a single higher level construct branch*ZeroOnSpeculate or something like that. It is I admit a bit wordsome but it would enable each architecure to implement it in its own way (For example both Mips and Aarch64 have the zero register thus not need the scratch for zero value etc.) Or maybe not event mention the zeroing. I think I recall that Aarch64 put some king of barrier for future proofing in case some some future uarch start speculation across the conditional moves, so you might want a some point add that.

Comment 23

[Dragan Mladjenovic]

Please ignore some parts of my post above that I thought I've posted hour ago. I'm now more in favor of solution in the post above it.

Comment 24

[qiaopengcheng]

Attachment: MIPS Implementation the Spectre functions

Comment 25

[qiaopengcheng]

Attachment: MIPS Implementation the Spectre functions

Comment 26

[Dragan Mladjenovic]

Comment on attachment 8989117 [details] [diff] [review]
MIPS Implementation the Spectre functions

Review of attachment 8989117 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MacroAssembler-inl.h
@@ +460,4 @@
>      branchPtr(cond, Address(scratch, ObjectGroup::offsetOfClasp()), ImmPtr(clasp), label);
>  
>      if (JitOptions.spectreObjectMitigationsMisc)
> +        spectreZeroRegister(cond, scratch, spectreRegToZero, SecondScratchReg, ScratchRegister);

I still have my reservations against expecting for ScratchRegister to be alive accros the branch. 
See https://searchfox.org/mozilla-central/source/js/src/jit/mips64/Assembler-mips64.cpp#235. 

I do not have a better idea that introduction some kind of branchPtr wrapper that on Mips will always leave 1/0 in SecondScrathReg and for other platforms the implementation would just delegate to branch. This way we would also remove the need for additional arguments for spectreZeroRegister. Still waiting for @jandem on his thoughts on this.

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +987,5 @@
> +    load32(addr, SecondScratchReg);
> +    ma_and(SecondScratchReg, SecondScratchReg, mask);
> +    ma_cmp_set(SecondScratchReg, SecondScratchReg, SecondScratchReg, cond);
> +    loadPtr(src, ScratchRegister);
> +    as_movn(dest, ScratchRegister, SecondScratchReg);

This implementation performs the load unconditionally then sets the dest to loaded value if the condition is satisfied.

I think that you should just have conditional load (what we already have) and that load should be spectre safe. Please refer to https://bug1442217.bmoattachments.org/attachment.cgi?id=8955102 for possible implementation.

@@ +994,5 @@
>  void
>  MacroAssembler::test32MovePtr(Condition cond, const Address& addr, Imm32 mask, Register src,
>                                Register dest)
>  {
> +    MOZ_ASSERT(cond == Assembler::Zero || cond == Assembler::NonZero);

Given implementation only works for cond == Assembler::Zero.

@@ +1009,4 @@
>      branch32(Assembler::BelowOrEqual, length, index, failure);
> +
> +    if (JitOptions.spectreIndexMasking) {
> +        ma_cmp_set(ScratchRegister, length, index, Assembler::BelowOrEqual);

This is ok, but you are unnecessary duplicating ma_cmp_set which is already part of branch32. The link I posted in some of previous comments has the implementation that avoids this duplication.

@@ +1023,5 @@
> +    load32(length, SecondScratchReg);
> +    branch32(Assembler::BelowOrEqual, SecondScratchReg, index, failure);
> +
> +    if (JitOptions.spectreIndexMasking) {
> +        ma_cmp_set(ScratchRegister, SecondScratchReg, index, Assembler::BelowOrEqual);

Same as previous.

Comment 27

[qiaopengcheng]

Attachment: MIPS Implementation the Spectre functions

Thanks for your reviw and suggestion.

> +    load32(length, SecondScratchReg);
> +    branch32(Assembler::BelowOrEqual, SecondScratchReg, index, failure);
> +
> +    if (JitOptions.spectreIndexMasking) {
> +        ma_cmp_set(ScratchRegister, SecondScratchReg, index, Assembler::BelowOrEqual);


Just for code funtion clare and calling relation is more simple.
And branch32 not always including the cmp* instruntion.

Comment 28

[Dragan Mladjenovic]

Comment on attachment 8989165 [details] [diff] [review]
MIPS Implementation the Spectre functions

Review of attachment 8989165 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/mips-shared/MacroAssembler-mips-shared-inl.h
@@ +988,5 @@
>      branchTest32(Assembler::InvertCondition(cond), addr, mask, &skip);
> +    load32(addr, SecondScratchReg);
> +    ma_and(SecondScratchReg, SecondScratchReg, mask);
> +    ma_cmp_set(SecondScratchReg, SecondScratchReg, SecondScratchReg, cond);
> +    loadPtr(src, ScratchRegister);

This load is still not Spectre safe. You need to "mask" the address before performing the load or maybe insert the speculation barrier.

@@ +1002,5 @@
> +    MOZ_ASSERT(cond == Assembler::Zero || cond == Assembler::NonZero);
> +
> +    load32(addr, SecondScratchReg);
> +    ma_and(SecondScratchReg, mask);
> +    ma_cmp_set(ScratchRegister, SecondScratchReg, SecondScratchReg, cond);

If you are only handling Zero/NonZero case you could drop the ma_cmp_set and use movz/movn.

@@ +1043,5 @@
> +    if (spectreCmpRegL == InvalidReg)
> +        spectreCmpRegL = SecondScratchReg;
> +
> +    if (spectreCmpRegR == InvalidReg)
> +        spectreCmpRegL = ScratchRegister;

You are still using the ScratchRegister that might be clobbered by branch before. So the same issue from previous review persists.

Comment 29

[qiaopengcheng]

Attachment: MIPS Implementation the Spectre functions

update the patch.
Maybe should wait the new definition interface of spectreZeroRegister/spectreMovePtr.

Comment 30

[qiaopengcheng]

(In reply to Dragan Mladjenovic from comment #28)

> 
> @@ +1043,5 @@
> > +    if (spectreCmpRegL == InvalidReg)
> > +        spectreCmpRegL = SecondScratchReg;
> > +
> > +    if (spectreCmpRegR == InvalidReg)
> > +        spectreCmpRegR = ScratchRegister;
> 
> You are still using the ScratchRegister that might be clobbered by branch
> before. So the same issue from previous review persists.

Now the function implementation of branchPtr on mips, it will not clobber the SecondScratchReg/ScratchRegister.
Of course as your comment #21, "This is a bit fragile because it relays that people using those methods pass the same Conditions as they passed to the branch before."

The functions spectreZeroRegister/spectreMovePtr relays on the function branchPtr.

Should I update the patch for reiew again ?
Or waiting [e-mail]  to refactor the functions spectreZeroRegister/spectreMovePtr ?

Comment 31

[Jan de Mooij [:jandem]]

Personally I still think we probably shouldn't bother for now with the Spectre mitigations that are hard to implement on MIPS.. (1) We have timer mitigations in place as well (2) The JIT mitigations were meant as a (temporary) stopgap until Project Fission (3) This code is subtle (mitigations are hard to test) and I'd prefer not making too many changes to these masm methods to avoid introducing regressions.

Comment 32

[Dragan Mladjenovic]

(In reply to qiaopengcheng from comment #30)
> (In reply to Dragan Mladjenovic from comment #28)
> 
> > 
> > @@ +1043,5 @@
> > > +    if (spectreCmpRegL == InvalidReg)
> > > +        spectreCmpRegL = SecondScratchReg;
> > > +
> > > +    if (spectreCmpRegR == InvalidReg)
> > > +        spectreCmpRegR = ScratchRegister;
> > 
> > You are still using the ScratchRegister that might be clobbered by branch
> > before. So the same issue from previous review persists.
> 
> Now the function implementation of branchPtr on mips, it will not clobber
> the SecondScratchReg/ScratchRegister.
> Of course as your comment #21, "This is a bit fragile because it relays that
> people using those methods pass the same Conditions as they passed to the
> branch before."
> 
> The functions spectreZeroRegister/spectreMovePtr relays on the function
> branchPtr.
> 
> Should I update the patch for reiew again ?
> Or waiting [e-mail]  to refactor the functions
> spectreZeroRegister/spectreMovePtr ?

Hi,

Sorry for the late replay. I'm more in favour that we  leave the spectreZeroRegister/spectreMovePtr as they are (w/o additional arguments) but instead replace  preceding uses of branchPtr with something like branchPtrSpectre? (killer part is the naming).

For non-MIPS branchPtrSpectre simply delagtes to branchPtr.

For MIPS: if cond is Zero , NonZero lhs is left in SecondScrathReg
          if cond is Equal, NonEqual  lhs xor rhs is left in SecondScrathReg
          otherwise the result of ma_cmp is left in SecondScrathReg

I haven't tried this, but I guess it is feasible.
Now spectreZeroRegister/spectreMovePtr can use SecondScratchReg.
Tagging @jandem for his thoughts on invasiveness of this approach.

Comment 33

[qiaopengcheng]

(In reply to Dragan Mladjenovic from comment #32)
> (In reply to qiaopengcheng from comment #30)
> > (In reply to Dragan Mladjenovic from comment #28)
> > 
> > > 
> > > @@ +1043,5 @@
> > > > +    if (spectreCmpRegL == InvalidReg)
> > > > +        spectreCmpRegL = SecondScratchReg;
> > > > +
> > > > +    if (spectreCmpRegR == InvalidReg)
> > > > +        spectreCmpRegR = ScratchRegister;
> > > 
> > > You are still using the ScratchRegister that might be clobbered by branch
> > > before. So the same issue from previous review persists.
> > 
> > Now the function implementation of branchPtr on mips, it will not clobber
> > the SecondScratchReg/ScratchRegister.
> > Of course as your comment #21, "This is a bit fragile because it relays that
> > people using those methods pass the same Conditions as they passed to the
> > branch before."
> > 
> > The functions spectreZeroRegister/spectreMovePtr relays on the function
> > branchPtr.
> > 
> > Should I update the patch for reiew again ?
> > Or waiting [e-mail]  to refactor the functions
> > spectreZeroRegister/spectreMovePtr ?
> 
> Hi,
> 
> Sorry for the late replay. I'm more in favour that we  leave the
> spectreZeroRegister/spectreMovePtr as they are (w/o additional arguments)
> but instead replace  preceding uses of branchPtr with something like
> branchPtrSpectre? (killer part is the naming).
> 
> For non-MIPS branchPtrSpectre simply delagtes to branchPtr.
> 
> For MIPS: if cond is Zero , NonZero lhs is left in SecondScrathReg
>           if cond is Equal, NonEqual  lhs xor rhs is left in SecondScrathReg
>           otherwise the result of ma_cmp is left in SecondScrathReg
> 
> I haven't tried this, but I guess it is feasible.
> Now spectreZeroRegister/spectreMovePtr can use SecondScratchReg.
> Tagging @jandem for his thoughts on invasiveness of this approach.



Thanks！

Maybe replacing the branchPtr is better when needing spectreZeroRegister/spectreMovePtr after the branchPtr.
Defining a specialized function, like branchPtrSpectre, for this issue.

Within the branchPtr or something like branchPtrSpectre, explicitly the rules of lhs and rhs registors.

Comment 34

[Jan de Mooij [:jandem]]

Sorry for the late response. The suggestion in comment 32 worries me because it introduces a hidden dependency between branchPtrSpectre and spectre* following it. Also instead of branchPtr we now have to use a spectre-specific version of it.

Unfortunately it looks like there isn't a great design here that doesn't complicate the code for all other platforms. In that case I'd prefer leaving things as is, I still don't think Spectre-MIPS is a real concern considering the small user base and our timer mitigations.

Comment 35

[qiaopengcheng]

(In reply to Jan de Mooij [:jandem] from comment #34)
> Sorry for the late response. The suggestion in comment 32 worries me because
> it introduces a hidden dependency between branchPtrSpectre and spectre*
> following it. Also instead of branchPtr we now have to use a
> spectre-specific version of it.
> 
> Unfortunately it looks like there isn't a great design here that doesn't
> complicate the code for all other platforms. In that case I'd prefer leaving
> things as is, I still don't think Spectre-MIPS is a real concern considering
> the small user base and our timer mitigations.


Now after downloading the inbound code, we must have to modify the code to disable the Spectre on mips-platform.
Otherwise it will segment-fault for Spectre default-enable.

What about disabling the Spectre by default on mips-platform?

diff --git a/js/src/jit/JitOptions.cpp b/js/src/jit/JitOptions.cpp
index 4a2a8af..e757f6c 100644
--- a/js/src/jit/JitOptions.cpp
+++ b/js/src/jit/JitOptions.cpp
@@ -234,12 +234,12 @@ DefaultJitOptions::DefaultJitOptions()
         }
     }
 
-    SET_DEFAULT(spectreIndexMasking, true);
-    SET_DEFAULT(spectreObjectMitigationsBarriers, true);
-    SET_DEFAULT(spectreObjectMitigationsMisc, true);
-    SET_DEFAULT(spectreStringMitigations, true);
-    SET_DEFAULT(spectreValueMasking, true);
-    SET_DEFAULT(spectreJitToCxxCalls, true);
+    SET_DEFAULT(spectreIndexMasking, false);
+    SET_DEFAULT(spectreObjectMitigationsBarriers, false);
+    SET_DEFAULT(spectreObjectMitigationsMisc, false);
+    SET_DEFAULT(spectreStringMitigations, false);
+    SET_DEFAULT(spectreValueMasking, false);
+    SET_DEFAULT(spectreJitToCxxCalls, false);
 
     // Toggles whether unboxed plain objects can be created by the VM.
     SET_DEFAULT(disableUnboxedObjects, false);
diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
index ac21b89..28eea5e 100644
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -6600,22 +6600,22 @@ JS_SetGlobalJitCompilerOption(JSContext* cx, JSJitCompilerOption opt, uint32_t v
         jit::JitOptions.disableOptimizationTracking = !value;
         break;
       case JSJITCOMPILER_SPECTRE_INDEX_MASKING:
-        jit::JitOptions.spectreIndexMasking = !!value;
+        //jit::JitOptions.spectreIndexMasking = !!value;
         break;
       case JSJITCOMPILER_SPECTRE_OBJECT_MITIGATIONS_BARRIERS:
-        jit::JitOptions.spectreObjectMitigationsBarriers = !!value;
+        //jit::JitOptions.spectreObjectMitigationsBarriers = !!value;
         break;
       case JSJITCOMPILER_SPECTRE_OBJECT_MITIGATIONS_MISC:
-        jit::JitOptions.spectreObjectMitigationsMisc = !!value;
+        //jit::JitOptions.spectreObjectMitigationsMisc = !!value;
         break;
       case JSJITCOMPILER_SPECTRE_STRING_MITIGATIONS:
-        jit::JitOptions.spectreStringMitigations = !!value;
+        //jit::JitOptions.spectreStringMitigations = !!value;
         break;
       case JSJITCOMPILER_SPECTRE_VALUE_MASKING:
-        jit::JitOptions.spectreValueMasking = !!value;
+        //jit::JitOptions.spectreValueMasking = !!value;
         break;
       case JSJITCOMPILER_SPECTRE_JIT_TO_CXX_CALLS:
-        jit::JitOptions.spectreJitToCxxCalls = !!value;
+        //jit::JitOptions.spectreJitToCxxCalls = !!value;
         break;
       case JSJITCOMPILER_WASM_FOLD_OFFSETS:
         jit::JitOptions.wasmFoldOffsets = !!value;
diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
index a5ffb44..ddd9b87 100644
--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -9852,12 +9852,13 @@ SetContextOptions(JSContext* cx, const OptionParser& op)
 
     if (const char* str = op.getStringOption("spectre-mitigations")) {
         if (strcmp(str, "on") == 0) {
-            jit::JitOptions.spectreIndexMasking = true;
-            jit::JitOptions.spectreObjectMitigationsBarriers = true;
-            jit::JitOptions.spectreObjectMitigationsMisc = true;
-            jit::JitOptions.spectreStringMitigations = true;
-            jit::JitOptions.spectreValueMasking = true;
-            jit::JitOptions.spectreJitToCxxCalls = true;
+            ;
+            //jit::JitOptions.spectreIndexMasking = true;
+            //jit::JitOptions.spectreObjectMitigationsBarriers = true;
+            //jit::JitOptions.spectreObjectMitigationsMisc = true;
+            //jit::JitOptions.spectreStringMitigations = true;
+            //jit::JitOptions.spectreValueMasking = true;
+            //jit::JitOptions.spectreJitToCxxCalls = true;
         } else if (strcmp(str, "off") == 0) {
             jit::JitOptions.spectreIndexMasking = false;
             jit::JitOptions.spectreObjectMitigationsBarriers = false;

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.