Support SGX extended product ID

[DL8]

KSS is a new feature that introduces additional fields to identify enclave in build time: ISVEXTPRODID and ISVFAMILYID. These fields are defined the the developer and signed in SIGSTRUCT. Add new fields to the manifest to add support, and support in SGX signing tool.

Add a hello world example with KSS enabled: Product ID fields are configured in the manifest and the enclave prints them. This example must be built with remote attestation.

If KSS is not supported on the platform, loading enclaves with KSS enabled will fail.

Signed-off-by: Lavi, Nir nir.lavi@intel.com

Description of the changes

The following fields were added to the manifest:

sgx.kss = [true|false] (default: false)
sgx.isvextprodid = "[16-byte hex value]"  (default: "0x00000000000000000000000000000000")
sgx.isvfamily    = "[16-byte hex value]"  (default: "0x00000000000000000000000000000000")

if sgx.kss = true, SGX signing tool will do the following in SIGSTRUCT:

1. Set attributes.kss = 1
2. Set isvextprodid and isvfamilyid` with values from the manifest

This is a partial fix for #800 . It does not address the following:

1. CONFIGID handling
2. RA-TLS verification callback changes

How to test this PR?

On a platform that supports KSS (indicated in is-sgx-available):

1. Go to the kss-helloworld example
2. Build it with SGX support: make SGX=1
3. Run the test script: ./run_test.sh 2>/dev/null

The output should be:

Running kss-helloworld
[ Success 1/4 ]
[ Success 2/4 ]
[ Success 3/4 ]
[ Success 4/4 ]

---

This change is 

[donporter]

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

a discussion (no related file):
You may wish to compare to/adopt the syntax of this PR: #881

---

a discussion (no related file):
I would prefer that the loader reject an application that requires KSS on an unsupported platform, versus a silent failure. Curious what others think.

---

---

CI-Examples/kss-helloworld/kss-helloworld.c line 33 at r1 (raw file):

        return 2;
    }
    if(strcmp(print_buffer, "none") == 0) {

We don't care what type, as long as it is not "none"?

---

CI-Examples/kss-helloworld/kss-helloworld.c line 59 at r1 (raw file):

    blob2hex(report.body.config_id, sizeof(report.body.config_id), print_buffer);
    printf("configid = %s\n", print_buffer);
    printf("configsvn = %d\n", report.body.config_svn);

It would be good if you wrapped this with a script that checked for the expected output that we can run in CI; apologies if I have missed this.

[DL8]

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @DL8, and @donporter)

a discussion (no related file):

Previously, donporter (Don Porter) wrote…

You may wish to compare to/adopt the syntax of this PR: #881

thanks for the reference. i looked at it, but i don't think this syntax is appropriate here, because KSS is more of an sgx-specific configuration than choice of cpu features. also, i don't see so much value in having an unspecified value for KSS

---

a discussion (no related file):

Previously, donporter (Don Porter) wrote…

I would prefer that the loader reject an application that requires KSS on an unsupported platform, versus a silent failure. Curious what others think.

loading on a platform without support will probably lead to a crash due to a reserved bit being set in SECS.ATTRIBUTES. this scenario is not covered in this PR

---

---

CI-Examples/kss-helloworld/kss-helloworld.c line 33 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

We don't care what type, as long as it is not "none"?

this example uses /dev/attestation/report, so we want ot make sure it's loaded. it will be loaded if remote attestation is enabled, but we don't really care which mechanism is used

---

CI-Examples/kss-helloworld/kss-helloworld.c line 59 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

It would be good if you wrapped this with a script that checked for the expected output that we can run in CI; apologies if I have missed this.

will do

[donporter]

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

---

CI-Examples/kss-helloworld/kss-helloworld.c line 33 at r1 (raw file):

Previously, DL8 (Nir) wrote…

this example uses /dev/attestation/report, so we want ot make sure it's loaded. it will be loaded if remote attestation is enabled, but we don't really care which mechanism is used

I see. Perhaps add a comment to this effect for future maintenance.

[DL8]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @donporter)

---

CI-Examples/kss-helloworld/kss-helloworld.c line 33 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

I see. Perhaps add a comment to this effect for future maintenance.

will do

[DL8]

Reviewable status: 4 of 13 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @donporter)

---

CI-Examples/kss-helloworld/kss-helloworld.c line 33 at r1 (raw file):

Previously, DL8 (Nir) wrote…

will do

Done.

---

CI-Examples/kss-helloworld/kss-helloworld.c line 59 at r1 (raw file):

Previously, DL8 (Nir) wrote…

will do

Done.

[DL8]

Reviewable status: 4 of 13 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @donporter)

a discussion (no related file):

Previously, DL8 (Nir) wrote…

loading on a platform without support will probably lead to a crash due to a reserved bit being set in SECS.ATTRIBUTES. this scenario is not covered in this PR

i tried loading an enclave with KSS on a platform without this feature. the result is ECREATE and the loader fails gracefully. is this behavior acceptable?

---

[donporter]

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @DL8)

a discussion (no related file):

Previously, DL8 (Nir) wrote…

i tried loading an enclave with KSS on a platform without this feature. the result is ECREATE and the loader fails gracefully. is this behavior acceptable?

Can you show an example of what you mean, by "the result is ECREATE" or sample output of the loader failing? I hope that the output gives the user a clear indication of what the problem is and how to learn more.

---

---

pal/src/host/linux-sgx/enclave_framework.c line 1194 at r1 (raw file):

    assert(stream->hdr.type == PAL_TYPE_PROCESS);

    memset(&target_info, 0, sizeof(target_info));

I worry that relying on get_current_enclave_target_info to always zero out all fields is prone to error, since it point-initializes fields and there could be an ambiguous expectation between caller and callee.

Assuming no other maintainers object, I would be ok with adding comments documenting an explicit pre/post condition that the caller need not initialize the tartet_info and that the post-condition is that all fields will be initialized, potentially to zero (and ensuring that this is what the current code does).

[DL8]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @donporter)

a discussion (no related file):

Previously, donporter (Don Porter) wrote…

Can you show an example of what you mean, by "the result is ECREATE" or sample output of the loader failing? I hope that the output gives the user a clear indication of what the problem is and how to learn more.

at the beginning i didn't have a platform without KSS, so i couldn't test this scenario. according to the SDM, ECREATE throws an exception in this case, so i assumed the loader would crash. however, now i have a machine without kss supported so i tried it and saw that it results in graceful failure. the output itself is not very informative:

$ gramine-sgx ./kss-helloworld
Gramine is starting. Parsing TOML manifest file, this may take some time...
error: ECREATE failed in enclave creation ioctl (errno = -5)
error: Creating enclave failed: -5
error: load_enclave() failed with error -5

---

---

pal/src/host/linux-sgx/enclave_framework.c line 1194 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

I worry that relying on get_current_enclave_target_info to always zero out all fields is prone to error, since it point-initializes fields and there could be an ambiguous expectation between caller and callee.

Assuming no other maintainers object, I would be ok with adding comments documenting an explicit pre/post condition that the caller need not initialize the tartet_info and that the post-condition is that all fields will be initialized, potentially to zero (and ensuring that this is what the current code does).

i think this discussion should be in a separate PR/issue

[donporter]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @DL8)

a discussion (no related file):

Previously, DL8 (Nir) wrote…

at the beginning i didn't have a platform without KSS, so i couldn't test this scenario. according to the SDM, ECREATE throws an exception in this case, so i assumed the loader would crash. however, now i have a machine without kss supported so i tried it and saw that it results in graceful failure. the output itself is not very informative:

$ gramine-sgx ./kss-helloworld
Gramine is starting. Parsing TOML manifest file, this may take some time...
error: ECREATE failed in enclave creation ioctl (errno = -5)
error: Creating enclave failed: -5
error: load_enclave() failed with error -5

Is there a way to make this message more informative?

---

---

pal/src/host/linux-sgx/enclave_framework.c line 1194 at r1 (raw file):

Previously, DL8 (Nir) wrote…

i think this discussion should be in a separate PR/issue

Perhaps in the interim we should not remove the memset, then?

[DL8]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @donporter)

a discussion (no related file):

Previously, donporter (Don Porter) wrote…

Is there a way to make this message more informative?

the error code can be translated with strerror(). a more informative solution will require adding an explicit check to the loader, but then why do we explicitly check this specific case but not others (e.g. an unsupported feature is requested, invalid launch token, invalid enclave state and so on)?

---

---

pal/src/host/linux-sgx/enclave_framework.c line 1194 at r1 (raw file):

Previously, donporter (Don Porter) wrote…

Perhaps in the interim we should not remove the memset, then?

looks like this memset was removed in commit 652d08af5d56a5396d707700a9c8ca43ba6c3dfb

[donporter]

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @DL8)

a discussion (no related file):

Previously, DL8 (Nir) wrote…

the error code can be translated with strerror(). a more informative solution will require adding an explicit check to the loader, but then why do we explicitly check this specific case but not others (e.g. an unsupported feature is requested, invalid launch token, invalid enclave state and so on)?

Ideally, we would check them all. I hate to burden you if this is a big ask; even a placeholder to slot in more informative messages for other cases would be an improvement.

---

---

pal/src/host/linux-sgx/enclave_framework.c line 1194 at r1 (raw file):

Previously, DL8 (Nir) wrote…

looks like this memset was removed in commit 652d08af5d56a5396d707700a9c8ca43ba6c3dfb

I see. I misunderstood the diff.

[DL8]

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @donporter)

a discussion (no related file):

Previously, donporter (Don Porter) wrote…

Ideally, we would check them all. I hate to burden you if this is a big ask; even a placeholder to slot in more informative messages for other cases would be an improvement.

do you think this PR is a good reference?
#1149

---

[donporter]

Reviewed 2 of 2 files at r4, 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: OSCAR Lab), "fixup! " found in commit messages' one-liners

a discussion (no related file):

Previously, DL8 (Nir) wrote…

do you think this PR is a good reference?
#1149

I think this is good. The newer messages are better.

---