Skip to content

Conversation

@jarkkojs
Copy link
Contributor

"user_vaddr" and "runtime_vaddr" in struct keystone_ioctl_enclave have no use. They are neither used by the SDK nor the driver. Remove them.

"user_vaddr" and "runtime_vaddr" in struct keystone_ioctl_enclave have no use.
They are neither used by the SDK nor the driver. Remove them.

Signed-off-by: Jarkko Sakkinen <[email protected]>
@dayeol
Copy link
Contributor

dayeol commented Oct 21, 2023

Thank you for the change!
FYI, we are currently planning for a big change in how enclave is initialized and booted. @evgenyp67 could you review this PR and keep your refactoring in sync, please?

@jarkkojs
Copy link
Contributor Author

Just generic advice, if you are refactoring it. You might also want to flatten the structure and use exact size type. I.e. it would be better to define runtime structure internally and do something similar I'm doing with the in-tree driver:

struct keystone_ioctl_enclave {
	__u64 eid;			/* out */
	__u64 min_pages;		/* in */
	__u64 runtime_vaddr;		/* not used by the driver */
	__u64 user_vaddr;		/* not used by the driver */
	__u64 pt_ptr;
	__u64 utm_free_ptr;
	__u64 epm_paddr;
	__u64 utm_paddr;
	__u64 runtime_paddr;
	__u64 user_paddr;
	__u64 free_paddr;
	__u64 epm_size;
	__u64 utm_size;
	__u64 runtime_entry;		/* in */
	__u64 user_entry;		/* in */
	__u64 untrusted_ptr;		/* in */
	__u64 untrusted_size;		/* in */
};

This is more or less an accepted practice in the upstream to keep uapi structures as flat as possible unless you have something special (e.g. array of things). It also helps with e.g. fuzz testing to have simple data structures.

The rationale for exact size type comes from having same ABI for both 32-bit and 64-bit builds so that a driver can be more robust with the user space.

@evgenyp67
Copy link
Contributor

Hoping to finish up the bulk of refactoring this-next week, this is what the direction is (will be removing elf and memory stuff from sdk also):
40d804e

This includes refactor changes allowed by the new in-enclave memory setup/ elf loading.

Will add you on the PR once I get there.

@jarkkojs
Copy link
Contributor Author

no rush

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