Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Illegal instruction" crash when doing base64 on x86_64 machines with AVX(2) support but "gather data sampling" mitigations enabled #50561

Closed
hardfalcon opened this issue Nov 5, 2023 · 8 comments

Comments

@hardfalcon
Copy link

Version

v21.1.0

Platform

Linux myhost 6.5.10-hardened1-1.1-hardened #1 SMP PREEMPT_DYNAMIC Sat, 04 Nov 2023 06:54:38 +0000 x86_64 GNU/Linux

Subsystem

base64

What steps will reproduce the bug?

On systems with a CPU that supports AVX or AVX2, but where the Linux kernel's "gather data sampling" mitigations are enabled (which then disables support for AVX, AVX2 and possibly SSE3), using somestring.toString("base64") leads to an illegal instruction, for example:

$ echo 'console.log(Buffer.from("test", "utf8").toString("base64"));' | node
Illegal instruction (core dumped)

Using gdb points towards the culprit being base64_stream_encode_avx2(), and I think this change would fix the issue.

How often does it reproduce? Is there a required condition?

See above.

What is the expected behavior? Why is that the expected behavior?

Node shouldn't crash but rather use a base64 routine that doesn't trigger an illegal instruction crash.

What do you see instead?

Node crashes with an illegal instruction.

Additional information

This also affects LTS releases of node, and other software using node, for example electron or lightdm-webkit2-greeter.

@hardfalcon
Copy link
Author

In case it helps: Upstream have merged the fix, so you could use simply use the corresponding commit from their repo.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 7, 2023

The problem is that base64 did not released a new version. So base64 0.5 is not containing the bugfix

@hardfalcon
Copy link
Author

hardfalcon commented Nov 7, 2023

I'm not the maintainer of that base64 library (of which you carry your own copy in your git tree anyway).

I've given you

  • a precise bug report,
  • a very concise description of how to reproduce the bug on the affected hardware,
  • a trivial patch that
    • has been merged by upstream,
    • and that fixes this obvious bug.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 7, 2023

And I gave you a reason why the automatic update of base64 in nodejs is not triggered, because the version did not change.

@marco-ippolito
Copy link
Member

marco-ippolito commented Nov 7, 2023

@hardfalcon until base64 releases a patch for this, it probably will not be fixed. you can try to apply the patch to our copy, but I'm not sure if it's going to be approved

@bnoordhuis
Copy link
Member

It's acceptable to float patches that have been merged upstream, it's just not preferable. I've asked upstream to make a release.

@bnoordhuis
Copy link
Member

Upstream did a release. I believe that means our auto-updater should pick it up Real Soon Now.

@lpinca lpinca closed this as completed in f45bb80 Nov 11, 2023
targos pushed a commit that referenced this issue Nov 11, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
targos pushed a commit that referenced this issue Nov 14, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@hardfalcon
Copy link
Author

In case anybody cares: This bug also affects older node versions (including the v18.x and v20.x branches, which contain the same base64 library.

UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit that referenced this issue Mar 20, 2024
PR-URL: #50629
Fixes: #50561
Fixes: #45091
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants