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

Remove redundant sgx.nonpie_binary manifest option #1187

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Feb 17, 2023

Description of the changes

It is now unclear why we needed this manifest option in the first place (probably to work around a bug in very old SGX drivers that prohibited mmapping the enclave space from address 0x0).

As a side effect, Gramine enclave base address is always 0x0.

TODOs (if this PR passes CI):

How to test this PR?

CI is enough (our CI has all kinds of SGX drivers which was the original problem iiuc).


This change is Reviewable

Copy link
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 53 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)

a discussion (no related file):
Btw, GSC always sets sgx.nonpie_binary = true, and we never experienced any problems (and noone every complained): https://github.com/gramineproject/gsc/blob/246423a6f8e49279dfb45ab53e8fb9423d1ae2da/templates/entrypoint.common.manifest.template#L15

So this is yet another argument that removing this manifest option just works, and nothing will break (GSC is heavily used by many folks, on different SGX drivers including legacy one).


@dimakuv
Copy link
Author

dimakuv commented Feb 17, 2023

Jenkins, retest pkg-rpm-almalinux9 please ( Curl error (6): Couldn't resolve host name for https://mirrors.almalinux.org/mirrorlist/9/appstream, unrelated)

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 42 of 53 files at r1.
Reviewable status: 42 of 53 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)


Documentation/devel/onboarding.rst line 268 at r1 (raw file):

     = true``. If you observe that memory addresses change constantly and hinder
     your debugging, set ``loader.insecure__disable_aslr = true``. But don't use
     the last two options in production; use them only for debugging and

these

Code quote:

the last

Copy link
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: 41 of 53 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), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)


Documentation/devel/onboarding.rst line 268 at r1 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

these

Done.

mkow
mkow previously approved these changes Feb 27, 2023
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 52 of 53 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)


pal/src/host/linux-sgx/host_main.c line 273 at r2 (raw file):

     * must cover code segment loaded at some hardcoded address (usually 0x400000), and heap cannot
     * start at zero (modern OSes do not allow this) */
    enclave->baseaddr = DEFAULT_ENCLAVE_BASE;

Actually, we should have probably done this long time ago as an anti-exploit mitigation: always having address 0 inside the enclave range makes it much harder to exploit NULL derefs, which are rather common.

It is now unclear why we needed this manifest option in the first place
(probably to work around a bug in very old SGX drivers that prohibited
mmapping the enclave space from address 0x0).

As a side effect, Gramine enclave base address is always 0x0.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Copy link
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: 50 of 53 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)


pal/src/host/linux-sgx/host_main.c line 273 at r2 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Actually, we should have probably done this long time ago as an anti-exploit mitigation: always having address 0 inside the enclave range makes it much harder to exploit NULL derefs, which are rather common.

Yes.

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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kailun-qin)

Copy link
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.

Dismissed @kailun-qin from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dimakuv dimakuv merged commit 38aa937 into master Feb 27, 2023
@dimakuv dimakuv deleted the dimakuv/rm-nonpie-binary branch February 27, 2023 18:48
haraldh added a commit to matter-labs/teepot that referenced this pull request Jul 2, 2024
see gramineproject/gramine#1187

Signed-off-by: Harald Hoyer <harald@matterlabs.dev>
haraldh added a commit to matter-labs/teepot that referenced this pull request Jul 2, 2024
see gramineproject/gramine#1187

Signed-off-by: Harald Hoyer <harald@matterlabs.dev>
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