WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
188145
Hardcoded LFENCE instruction
https://bugs.webkit.org/show_bug.cgi?id=188145
Summary
Hardcoded LFENCE instruction
karogyoker2+webkit
Reported
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.
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
,
EWS Watchlist
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
,
EWS Watchlist
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
,
EWS Watchlist
no flags
Details
Patch
(3.62 KB, patch)
2018-08-04 22:54 PDT
,
karogyoker2+webkit
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
karogyoker2+webkit
Comment 1
2018-07-29 01:54:01 PDT
Created
attachment 346021
[details]
Patch
karogyoker2+webkit
Comment 2
2018-07-29 02:01:04 PDT
***
Bug 187889
has been marked as a duplicate of this bug. ***
Yusuke Suzuki
Comment 3
2018-07-29 10:45:49 PDT
Then, what makes WebKit safe from Spectre?
EWS Watchlist
Comment 4
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
EWS Watchlist
Comment 5
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
karogyoker2+webkit
Comment 6
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).
Yusuke Suzuki
Comment 7
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
karogyoker2+webkit
Comment 8
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
karogyoker2+webkit
Comment 9
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
karogyoker2+webkit
Comment 10
2018-07-30 08:42:44 PDT
Created
attachment 346059
[details]
Patch
Filip Pizlo
Comment 11
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.
karogyoker2+webkit
Comment 12
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
EWS Watchlist
Comment 13
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
EWS Watchlist
Comment 14
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
karogyoker2+webkit
Comment 15
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?
Filip Pizlo
Comment 16
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().
karogyoker2+webkit
Comment 17
2018-08-04 08:07:02 PDT
Created
attachment 346589
[details]
Patch
EWS Watchlist
Comment 18
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
EWS Watchlist
Comment 19
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
Michael Catanzaro
Comment 20
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.
Michael Catanzaro
Comment 21
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.
Michael Catanzaro
Comment 22
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.
karogyoker2+webkit
Comment 23
2018-08-04 22:54:56 PDT
Created
attachment 346598
[details]
Patch
karogyoker2+webkit
Comment 24
2018-08-07 04:51:56 PDT
Do I have anything else to do to have this committed?
Michael Catanzaro
Comment 25
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)
WebKit Commit Bot
Comment 26
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
>
WebKit Commit Bot
Comment 27
2018-08-07 05:50:33 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug