Bug 188145 - Hardcoded LFENCE instruction
Summary: Hardcoded LFENCE instruction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P1 Blocker
Assignee: Nobody
URL:
Keywords:
: 187889 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-07-29 01:40 PDT by karogyoker2+webkit
Modified: 2018-08-07 05:50 PDT (History)
15 users (show)

See Also:


Attachments
crashlog (1.51 MB, text/plain)
2018-07-29 01:40 PDT, karogyoker2+webkit
no flags Details
Patch (1019 bytes, patch)
2018-07-29 01:54 PDT, karogyoker2+webkit
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (13.03 MB, application/zip)
2018-07-29 11:04 PDT, Build Bot
no flags Details
Patch (1.13 KB, patch)
2018-07-30 08:42 PDT, karogyoker2+webkit
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.96 MB, application/zip)
2018-07-30 15:44 PDT, Build Bot
no flags Details
Patch (3.55 KB, patch)
2018-08-04 08:07 PDT, karogyoker2+webkit
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.93 MB, application/zip)
2018-08-04 11:02 PDT, Build Bot
no flags Details
Patch (3.62 KB, patch)
2018-08-04 22:54 PDT, karogyoker2+webkit
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description karogyoker2+webkit 2018-07-29 01:40:47 PDT
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.
Comment 1 karogyoker2+webkit 2018-07-29 01:54:01 PDT
Created attachment 346021 [details]
Patch
Comment 2 karogyoker2+webkit 2018-07-29 02:01:04 PDT
*** Bug 187889 has been marked as a duplicate of this bug. ***
Comment 3 Yusuke Suzuki 2018-07-29 10:45:49 PDT
Then, what makes WebKit safe from Spectre?
Comment 4 Build Bot 2018-07-29 11:04:00 PDT
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
Comment 5 Build Bot 2018-07-29 11:04:12 PDT
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
Comment 6 karogyoker2+webkit 2018-07-29 11:49:59 PDT
(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).
Comment 7 Yusuke Suzuki 2018-07-29 12:49:27 PDT
(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
Comment 8 karogyoker2+webkit 2018-07-29 19:59:33 PDT
> 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
Comment 9 karogyoker2+webkit 2018-07-29 20:26:49 PDT
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
Comment 10 karogyoker2+webkit 2018-07-30 08:42:44 PDT
Created attachment 346059 [details]
Patch
Comment 11 Filip Pizlo 2018-07-30 08:52:34 PDT
(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.
Comment 12 karogyoker2+webkit 2018-07-30 09:23:58 PDT
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 13 Build Bot 2018-07-30 15:44:21 PDT
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
Comment 14 Build Bot 2018-07-30 15:44:33 PDT
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
Comment 15 karogyoker2+webkit 2018-08-03 10:12:27 PDT
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 16 Filip Pizlo 2018-08-03 10:15:33 PDT
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().
Comment 17 karogyoker2+webkit 2018-08-04 08:07:02 PDT
Created attachment 346589 [details]
Patch
Comment 18 Build Bot 2018-08-04 11:01:57 PDT
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
Comment 19 Build Bot 2018-08-04 11:02:09 PDT
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 20 Michael Catanzaro 2018-08-04 11:22:07 PDT
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.
Comment 21 Michael Catanzaro 2018-08-04 11:47:03 PDT
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.
Comment 22 Michael Catanzaro 2018-08-04 13:32:19 PDT
(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.
Comment 23 karogyoker2+webkit 2018-08-04 22:54:56 PDT
Created attachment 346598 [details]
Patch
Comment 24 karogyoker2+webkit 2018-08-07 04:51:56 PDT
Do I have anything else to do to have this committed?
Comment 25 Michael Catanzaro 2018-08-07 05:23:03 PDT
Comment on attachment 346598 [details]
Patch

You can set the cq? flag in Bugzilla next time (commit-queue request)
Comment 26 WebKit Commit Bot 2018-08-07 05:50:31 PDT
Comment on attachment 346598 [details]
Patch

Clearing flags on attachment: 346598

Committed r234648: <https://trac.webkit.org/changeset/234648>
Comment 27 WebKit Commit Bot 2018-08-07 05:50:33 PDT
All reviewed patches have been landed.  Closing bug.