1866545 - Crash [@ ??] with wasm module on 32-bit

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20231124-dbd1a6aef0a7 (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

See attachment.

Backtrace:

received signal SIGILL, Illegal instruction.
#0  0x33514063 in ?? ()
#1  0x335141e3 in ?? ()
#2  0xf3400708 in ?? ()
eax	0x0	0
ebx	0x0	0
ecx	0xf3400708	-213907704
edx	0x0	0
esi	0xf6ef8750	-152074416
edi	0xdeadbeef	-559038737
ebp	0xffffbfa8	4294950824
esp	0xffffbf90	4294950800
eip	0x33514063	860962915
=> 0x33514063:	ud2    
   0x33514065:	mov    0x10(%ecx),%edx

This only reproduces on 32-bit.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1866545 using build mozilla-central 20231124092418-dbd1a6aef0a7. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: test.js

Comment 5

[Ryan Hunt [:rhunt]]

I'm able to reproduce this. I minimized the test case and attached it. This looks tail calls related, removing the final return_call left in the test case causes it to not crash.

Comment 6

[Ryan Hunt [:rhunt]]

Here is the disassembly I'm seeing for the relevant functions on x86.

--------------------------------------------------
Kind = Function, index = 0, name = trap:

00000000  55                                    push %rbp
00000001  8b ec                                 mov %esp, %ebp
00000003  0f 0b                                 ud2
00000005  5d                                    pop %rbp
00000006  c3                                    ret

--------------------------------------------------
Kind = Function, index = 1, name = second:

00000000  55                                    push %rbp
00000001  8b ec                                 mov %esp, %ebp
00000003  83 ec 18                              sub $0x18, %esp
00000006  39 66 1c                              cmpl %esp, 0x1C(%rsi)
00000009  0f 82 02 00 00 00                     jb 0x0000000000000011
0000000F  0f 0b                                 ud2
00000011  c7 44 24 08 21 00 00 00               movl $0x21, 0x08(%rsp)
00000019  c7 44 24 0c 42 00 00 00               movl $0x42, 0x0C(%rsp)
00000021  f7 c4 0f 00 00 00                     test $0x0F, %esp
00000027  0f 84 01 00 00 00                     jz 0x000000000000002E
0000002D  cc                                    int3
0000002E  51                                    push %rcx
0000002F  8b 55 00                              movl (%rbp), %edx
00000032  8b 4d 04                              movl 0x04(%rbp), %ecx
00000035  50                                    push %rax
00000036  8b 45 f4                              movl -0x0C(%rbp), %eax
00000039  89 45 14                              movl %eax, 0x14(%rbp)
0000003C  8b 45 f0                              movl -0x10(%rbp), %eax
0000003F  89 45 10                              movl %eax, 0x10(%rbp)
00000042  58                                    pop %rax
00000043  89 75 08                              movl %esi, 0x08(%rbp)
00000046  89 4d 04                              movl %ecx, 0x04(%rbp)
00000049  8b 1c 24                              movl (%rsp), %ebx
0000004C  89 0c 24                              movl %ecx, (%rsp)
0000004F  8b cb                                 mov %ebx, %ecx
00000051  83 c4 20                              add $0x20, %esp
00000054  8b ea                                 mov %edx, %ebp
00000056  e9 95 ff ff ff                        jmp 0xFFFFFFFFFFFFFFF0
0000005B  83 c4 18                              add $0x18, %esp
0000005E  5d                                    pop %rbp
0000005F  c3                                    ret

Comment 7

[Yury Delendik (:yury)]

Attachment: Bug 1866545 - Simplify stack adjustments in Ion. r?rhunt

Comment 8

[Daniel Veditz [:dveditz]]

This particular example in comment 0 looks like it's about to safely crash on a null deref if the ud2 weren't there, but worried that more generically if we're wrong with tail-call alignment something could be worse with a different testcase.

Comment 9

[Christian Holler (:decoder)]

I'm upping this to sec-high instead because this is an alignment issue and it's unlikely to always crash on ud2. Per default policy, such crashes are sec-high unless they require the debugger etc, which is not the case here. If :yury thinks this will always be a safe crash, we should remove the sec keyword altogether. Lets stop using sec-moderate for these "maybe sec-high" bugs.

Comment 10

[Donal Meehan [:dmeehan]]

Setting Bug 1862473 as the regressor, please correct if needed

Comment 11

[Ryan Hunt [:rhunt]]

(In reply to Christian Holler (:decoder) from comment #9)

I'm upping this to sec-high instead because this is an alignment issue and it's unlikely to always crash on ud2. Per default policy, such crashes are sec-high unless they require the debugger etc, which is not the case here. If :yury thinks this will always be a safe crash, we should remove the sec keyword altogether. Lets stop using sec-moderate for these "maybe sec-high" bugs.

I agree with this, I think this could result in parts of our stack frames (such as return addresses) being leaked as locals or corrupted.

Comment 12

[Yury Delendik (:yury)]

Comment on attachment 9366118 [details]
Bug 1866545 - Simplify stack adjustments in Ion. r?rhunt

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, but gives an idea it is connected with new tail calls functionality, can narrow fuzzing.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 121
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It is covered by automated testing well.
• Is Android affected?: Yes

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9366118 [details]
Bug 1866545 - Simplify stack adjustments in Ion. r?rhunt

approved to land and request uplift

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c36e4fdf90fd
Simplify stack adjustments in Ion. r=rhunt

Comment 15

[Yury Delendik (:yury)]

Comment on attachment 9366118 [details]
Bug 1866545 - Simplify stack adjustments in Ion. r?rhunt

Beta/Release Uplift Approval Request

• User impact if declined: Crashes with certain wasm tail calls programs
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Affected code well-covered by automated testing. Affects Wasm programs
• String changes made/needed:
• Is Android affected?: Unknown

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c36e4fdf90fd

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366118 [details]
Bug 1866545 - Simplify stack adjustments in Ion. r?rhunt

Approved for 121.0b8.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/612721b93f60

Comment 19

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

2 months ago, yury placed a reminder on the bug using the whiteboard tag [reminder-test 2024-02-07] .

yury, please refer to the original comment to better understand the reason for the reminder.

Comment 20

[Yury Delendik (:yury)]

Attachment: Bug 1866545 - Add wasm tail call test. r?jseward

Comment 21

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

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75727deb4d0b
Add wasm tail call test. r=jseward

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75727deb4d0b