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

[PAL] Fix memory PAL regression test for UBSan #2003

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Sep 24, 2024

Description of the changes

Newer Clang versions (v18 and newer) added more UBSan checks that triggered UBSan on PAL regression tests. In particular, UBSan added the check "Indirect call of a function through a function pointer of the wrong type". UBSan expects all functions which can be indirectly called to be instrumented with two magic metadata values, located right-before the function in address space.

The memory test however does not add this required metadata to a dummy generated-on-the-fly function (it simply allocates some pages and copies the asm code at the beginning of the first page). Instead of adding UBSan-required metadata, we simply disable this particular check (it's a test after all, not core Gramine functionality).

References:

Reported by @woju.

How to test this PR?

Run PAL regression tests on Ubuntu 24.04 + UBSan enabled.


This change is Reviewable

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


-- commits line 4 at r1:
Isn't it v17? (according to https://maskray.me/blog/2022-12-18-control-flow-integrity#fsanitizefunction)

Code quote:

v18

-- commits line 4 at r1:
Isn't it (also) because since v17-fsanitize=function was allowed on C code, while previously it was C++ only (based on https://reviews.llvm.org/D148827)?

Code quote:

added more UBSan checks

@dimakuv dimakuv force-pushed the dimakuv/fix-ubsan-pal-regr-memory branch from 75d6f50 to 9777dac Compare September 24, 2024 10:06
Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)


-- commits line 4 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

Isn't it v17? (according to https://maskray.me/blog/2022-12-18-control-flow-integrity#fsanitizefunction)

Done.


-- commits line 4 at r1:

Previously, kailun-qin (Kailun Qin) wrote…

Isn't it (also) because since v17-fsanitize=function was allowed on C code, while previously it was C++ only (based on https://reviews.llvm.org/D148827)?

Done. Rephrased.

kailun-qin
kailun-qin previously approved these changes Sep 24, 2024
Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)


pal/regression/memory.c line 96 at r2 (raw file):

 * to be instrumented with two magic metadata values, located right-before the function in address
 * space. The below code does *not* add metadata (it simply allocates some pages and copies the asm
 * code at the beginning of the first page). Instead of adding UBSan-required metadata, we simply

"asm code" -> "machine code"

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)


pal/regression/memory.c line 96 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

"asm code" -> "machine code"

Done. Also in the commit message.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Newer Clang versions (v17 and newer) added more UBSan checks for C code
that started triggering UBSan on PAL regression tests. In particular,
UBSan added the check "Indirect call of a function through a function
pointer of the wrong type". UBSan expects all functions which can be
indirectly called to be instrumented with two magic metadata values,
located right-before the function in address space.

The `memory` test however does *not* add this required metadata to a
dummy generated-on-the-fly function (it simply allocates some pages and
copies machine code at the beginning of the first page). Instead of
adding UBSan-required metadata, we simply disable this particular check
(it's a test after all, not core Gramine functionality).

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@mkow mkow force-pushed the dimakuv/fix-ubsan-pal-regr-memory branch from 0f22fa7 to bf765c8 Compare September 26, 2024 17:19
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit bf765c8 into master Sep 26, 2024
18 checks passed
@dimakuv dimakuv deleted the dimakuv/fix-ubsan-pal-regr-memory branch September 26, 2024 18:37
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 this pull request may close these issues.

3 participants