Created attachment 346020 [details] crashlog WebKit crashes instantly on all 32 bit AMD processors and also on all 32 bit Intel processors (except Willamette or newer) since 2.19.90 (released on February 5, 2018). This is because there is a hardcoded LFENCE instruction which can be only run by CPUs which have the SSE2 instruction set.
Created attachment 346021 [details] Patch
*** Bug 187889 has been marked as a duplicate of this bug. ***
Then, what makes WebKit safe from Spectre?
Comment on attachment 346021 [details] Patch Attachment 346021 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8692208 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 346034 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Yusuke Suzuki from comment #3) > Then, what makes WebKit safe from Spectre? Good point. I was thinking a vulnerable browser is still better than a browser which is completely unusable. But obviously, this reasoning is not security oriented. Then maybe we should fall back to generic retpoline here, if it is possible. Since my C++ knowledge ends here, sadly, I cannot help on this. Maybe we should look into how Mozilla fixed this, because Firefox ESR 52 has Spectre mitigation and still supported until the end of August. Also, it doesn't require SSE2 (on Linux). Or just release an official statement that machines without SSE2 are no longer supported (like Google or Mozilla did).
(In reply to karogyoker2+webkit from comment #6) > (In reply to Yusuke Suzuki from comment #3) > > Then, what makes WebKit safe from Spectre? > > Good point. I was thinking a vulnerable browser is still better than a > browser which is completely unusable. But obviously, this reasoning is not > security oriented. > Then maybe we should fall back to generic retpoline here, if it is possible. > Since my C++ knowledge ends here, sadly, I cannot help on this. Maybe we > should look into how Mozilla fixed this, because Firefox ESR 52 has Spectre > mitigation and still supported until the end of August. Also, it doesn't > require SSE2 (on Linux). Firefox ESR 52 includes Firefox 49. So it does not work w/o SSE2[1]. > > Or just release an official statement that machines without SSE2 are no > longer supported (like Google or Mozilla did). It seems that current supported Firefox and Chromium no longer support x86 w/o SSE2. Windows 7 also does not support it now[2]. So I think dropping support for x86 w/o SSE2 is good solution in this case. I've sent a mail to webkit-dev mailing list to clarify that WebKit does not support non-SSE2 x86[3]. [1]: https://support.mozilla.org/en-US/kb/your-hardware-no-longer-supported [2]: https://support.microsoft.com/en-us/help/4088875/windows-7-update-kb4088875 [3]: https://lists.webkit.org/pipermail/webkit-dev/2018-July/030071.html
> Firefox ESR 52 includes Firefox 49. So it does not work w/o SSE2[1]. It DOES work on Linux. Firefox ESR 52 does NOT require SSE2 on Linux. The first version of FF for Linux which require SSE2 is version 53.0. Here is the official statement: https://www.mozilla.org/en-US/firefox/53.0/releasenotes/ "Ended Firefox Linux support for processors older than Pentium 4 and AMD Opteron" I can also prove that if works without SSE2 because that is what I use on my Athlon XP. Also here are some links which prove that people are running FF 52 ESR for Linux on machines without SSE2: https://forum.manjaro.org/t/testing-update-x32-2018-04-09-to-11-firefox-sse-firefox-esr-libreoffice-fresh-systemd-kernels-and-extramodules/44439 https://www.linuxliteos.com/forums/installing-software/cpu-without-sse2-crashes-firefox-update/ https://ubuntuforums.org/showthread.php?t=2382079
I wouldn't mention the Windows 7 example here, because in case of Firefox or Chromium they made a statement and they ended support for machines without SSE2 on purpose but in the Windows 7 case they made a programming mistake when mitigating the Spectre vulnerability, so it is a bug. https://www.askwoody.com/2018/pentium-iii-users-knocked-out-of-win7-patches/ https://www.gizmodo.com.au/2018/06/microsoft-quietly-drops-support-for-non-sse2-cpus-in-windows-7/ Microsoft also had problems with CPUs which have SSE2 but Windows couldn't boot anymore after the update: https://www.computerworld.com/article/3247676/microsoft-windows/microsofts-mystifying-meltdownspectre-patches-for-amd-processors.html https://www.theverge.com/2018/1/9/16867068/microsoft-meltdown-spectre-security-updates-amd-pcs-issues
Created attachment 346059 [details] Patch
(In reply to Yusuke Suzuki from comment #3) > Then, what makes WebKit safe from Spectre? I don't think that the lfence instruction is adequate for protecting against Spectre anyway.
I've created a new patch. Now it has Spectre mitigation also for machines without SSE2. It is using the slower CPUID instruction instead of LFENCE, but it is still better than an instant app crash. My idea of withdrawing support for non-SSE2 X86 machines was just a quick, not well thought idea and I already regretted that I mentioned it. Please, don't make obsolete those machines if it is possible to fix this issue without obsoleting a lot of CPUs. Let me quote Michael Catanzaro, developer at Igalia[1]: "For WebKitGTK+, SSE2 instructions are forbidden (except when building for x86_64) because that's what our distributors require. But I doubt all developers are aware of this, and I also doubt anybody ever tests on such old hardware. So it might require some effort to audit the codebase for unwanted SSE2 instructions to make sure they're not there and fix them if so." So, it isn't really an option to not support non-SSE2 machines. Also, even if there is no Spectre mitigation for some cases, it can be OK. For example, in case of a Windows build, the LFENCE instruction is not emitted[2]. Furthermore, there are no Spectre mitigations for MIPS (even though some newer MIPS processors are affected too[3]), but let me correct if I'm wrong. There are also controversies if LFENCE is "adequate enough for protecting against Spectre"[4]. Personally, I believe that the reasoning that WebKit can abandon non-SSE2 users because Microsoft also abandoned them (despite their product's end of life has not come yet), is not valid. Especially that they didn't do it on purpose. Additionally, Linux still cares about non-SSE2 users because if it cannot detect a serializing LFENCE instruction in the CPU then it switches to full generic retpoline (at it can be seen in the attached crashlog). Theoretically, if there would be a decision to abandon non-SSE2 CPUs, it would be still needed to give a proper error message to the user instead of an app crash. But this would also require development, it would be easier to just use my patch (if it is accepted). [1]: https://bugs.webkit.org/show_bug.cgi?id=187701#c7 [2]: https://bugs.webkit.org/show_bug.cgi?id=188149 [3]: https://www.mips.com/forums/topic/mips-mitigations-for-side-channel-vulnerabilities-on-speculative-execution-cpus/ [4]: https://bugs.webkit.org/show_bug.cgi?id=188145#c11
Comment on attachment 346059 [details] Patch Attachment 346059 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8703121 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html legacy-animation-engine/imported/blink/transitions/unprefixed-transform.html
Created attachment 346112 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Can somebody please merge this? It is very critical for me and the patch is very trivial. Or is there any problem with it?
Comment on attachment 346059 [details] Patch This change is wrong because cpuid is dramatically slower than lfence and because lfence is no longer the way we are mitigating Spectre. We should just remove all uses of speculationFence() and remove speculationFence().
Created attachment 346589 [details] Patch
Comment on attachment 346589 [details] Patch Attachment 346589 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8762497 New failing tests: http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html http/tests/security/canvas-remote-read-remote-video-redirect.html http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 346591 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346589&action=review > Source/JavaScriptCore/ChangeLog:4 > + Remove lfence instruction because it is crashing systems without SSE2 and > + this is not the way how WebKit mitigates Spectre. Here you should just put the title of the bug > Source/JavaScriptCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + * runtime/JSLock.cpp: Then move the change description down here, between the Reviewed by line and the changelist.
Technically, many of WebKitGTK+'s distributors require that it not use SSE2 instructions in 32-bit builds. I am no longer sure which ones. (It used to be the case for Fedora, for example, but that changed recently.) In practice, so few people care about these old machines anymore that such use is unlikely to be noticed by many users. Certain software (including Firefox and Chromium) has already started to require SSE2 and is broken on these machines, so the requirement is already not fulfilled by other software. E.g. it is clear that Debian is not patching Firefox or Chromium to remove the SSE2 instructions. So times change, and I think we can get away with starting to require SSE2, if there is a compelling reason to do so. But according to Filip's review in that bug, we probably don't need to do so right now.
(In reply to Michael Catanzaro from comment #21) > So times change, and I think we can get away with starting to require SSE2, > if there is a compelling reason to do so. But according to Filip's review in > that bug, we probably don't need to do so right now. So to be clear: I'm OK with this patch, given that it implements Filip's request exactly. Just the format of the ChangeLog file needs to be fixed.
Created attachment 346598 [details] Patch
Do I have anything else to do to have this committed?
Comment on attachment 346598 [details] Patch You can set the cq? flag in Bugzilla next time (commit-queue request)
Comment on attachment 346598 [details] Patch Clearing flags on attachment: 346598 Committed r234648: <https://trac.webkit.org/changeset/234648>
All reviewed patches have been landed. Closing bug.