Skip to content

Conversation

@Niederb
Copy link
Contributor

@Niederb Niederb commented Dec 1, 2022

This PR is more a proposal on how to improve our handling of unsafe code.

  • I added/removed some unsafe depending on if the function has unsafe code. Also see Consistent marking of functions as unsafe #982
  • The more interesting part is in get_update_info.rs. Currently both methods ocall_get_update_info and get_update_info have unsafe code. I refactored it such that one function is safe and the other unsafe. The safe one also has a more rust like interface.
    • The parameters are quite easy to convert. The return value and error handling is a bit more involved but still doable.
  • Also added nullptr checks in get_update_info.rs. See Check safety of from_raw_parts and from_raw_parts_mut #1113
  • I think we should also already consider RFC-2585: No longer treat the body of an unsafe fn as being an unsafe block.

@Niederb
Copy link
Contributor Author

Niederb commented Dec 1, 2022

@murerfel Maybe you also want to have a look

Comment on lines +30 to +32
if p_platform_blob.is_null() || p_update_info.is_null() {
return sgx_status_t::SGX_ERROR_INVALID_PARAMETER
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I think I like your proposal, and agree with it.

The only thing that I wonder; the teaclave examples don't check for null pointers in their sample code either, so I wonder if the overall construct with rust FFI and .edl files gives guarantees such that these function args can't be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yeah you are right regarding the teaclave example, but I don't see how they could guarantee that the arguments are never null. It can be a valid use-case to pass a null into a function so it cannot be enforced in general.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could imagine that the edger8r does enforce it. As the .edl files are strongly typed, the edger8r might enforce that a pointer to something does, in fact, always point to a valid object of said type. I am not sure about this though, but I found this very interesting reference to the edger8r.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is only one place in the code samples where they check for null pointer on the v2.0.0-preview branch:
https://github.com/apache/incubator-teaclave-sgx-sdk/blob/v2.0.0-preview/samplecode/httpreq/enclave/src/lib.rs#L33:L38

(There are some checks on the current master branch as well)


I did not find anything related to explicit null checking in https://download.01.org/intel-sgx/latest/linux-latest/docs/Intel_SGX_Developer_Reference_Linux_2.18_Open_Source.pdf

I think it is worth to asking it on openenclave though just to have a confirmation.

@OverOrion OverOrion removed their request for review October 30, 2023 16:14
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.

4 participants