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] Refactor pal_handle->generic.fd field #1989

Open
dimakuv opened this issue Sep 2, 2024 · 0 comments
Open

[PAL] Refactor pal_handle->generic.fd field #1989

dimakuv opened this issue Sep 2, 2024 · 0 comments
Labels
enhancement New feature or request P: 3

Comments

@dimakuv
Copy link

dimakuv commented Sep 2, 2024

PAL handles have a pal_handle->generic.fd field, which is used in "generic" (not PAL-type-specific) code. Here's the declaration:

/* Common field for accessing underlying host fd. See also `PAL_HANDLE_FD_READABLE`. */
struct {
PAL_IDX fd;
} generic;

Whether this generic fd field is used, can be indirectly deduced from pal_handle->flags. We have this comment at the flags declaration:

/* These two flags indicate whether the underlying host fd of `PAL_HANDLE` is readable and/or
* writable respectively. If none of these is set, then the handle has no host-level fd. */
#define PAL_HANDLE_FD_READABLE 1
#define PAL_HANDLE_FD_WRITABLE 2

Most type-specific PAL handles have the first field that aliases this generic.fd, here's a random example for the "console" type:

struct {
PAL_IDX fd;
} console;

Some type-specific PAL handles do not have the first field that aliases this generic.fd, here's an example ("event" type):

struct {
/* Guards accesses to the rest of the fields.
* We need to be able to set `signaled` and `signaled_untrusted` atomically, which is
* impossible without a lock. They are essentialy the same field, but we need two
* separate copies, because we need to guard against malicious host modifications yet
* still be able to call futex on it. */
spinlock_t lock;
/* Current number of waiters - used solely as an optimization. `uint32_t` because futex
* syscall does not allow for more than `INT_MAX` waiters anyway. */
uint32_t waiters_cnt;
bool signaled;
bool auto_clear;
/* Access to the *content* of this field should be atomic, because it's used as futex
* word on the untrusted host. We use 8-byte ints instead of classic 4-byte ints for
* this futex word. This is to mitigate CVE-2022-21166 (INTEL-SA-00615) which requires
* all writes to untrusted memory from within the enclave to be done in 8-byte chunks
* aligned to 8-bytes boundary. We can safely typecast this 8-byte int to a 4-byte futex
* word because Intel SGX implies a little-endian CPU. */
uint64_t* signaled_untrusted;
} event;

Finally, to use pal_handle->generic.fd, one needs to condition on the flags field, like this:

.has_fd = cargo->flags & (PAL_HANDLE_FD_READABLE | PAL_HANDLE_FD_WRITABLE),


This is a brittle and weird design. Needs to be refactored.

This issue was extracted from a discussion https://reviewable.io/reviews/gramineproject/gramine/1812#-O4dzDjGAdtVeJOUybN_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P: 3
Projects
None yet
Development

No branches or pull requests

2 participants