[common] Refactor Protected Files, part 5

[dimakuv]

Description of the changes

This commit refactors PF code without changing functionality (part 5 in a series of commits). In particular, this commit moves code around, rephrases some comments, removes useless comments and empty lines, as well as renames vague data and block words to more specific node.

This PR supersedes #1894.

How to test this PR?

N/A.

---

This change is 

[mkow]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 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)

---

common/src/protected_files/protected_files.c line 188 at r1 (raw file):

}

static bool ipf_generate_nonced_metadata_key(pf_context_t* pf, pf_key_t* output) {

I preferred the old name, "generating a key" usually means some randomness. Maybe this way?

Suggestion:

ipf_recreate_metadata_key

---

common/src/protected_files/protected_files.c line 223 at r1 (raw file):

crypto slot of MHT node

What do you mean by that?

---

common/src/protected_files/protected_files.c line 415 at r1 (raw file):

    pf_key_t key;

    // new key for metadata node encryption, saves the key nonce in metadata plaintext header

Could you do that gluing of "meta" and "data" also in all other places in PF code?

Code quote:

metadata

---

common/src/protected_files/protected_files.c line 421 at r1 (raw file):

    }

    // encrypt metadata part-to-be-encrypted, also updates the MAC in metadata plaintext header

Suggestion:

// encrypt metadata part-to-be-encrypted, also updating the MAC in metadata plaintext header

---

common/src/protected_files/protected_files.c line 550 at r1 (raw file):

    while (lruc_size(pf->cache) > MAX_NODES_IN_CACHE) {
        void* node = lruc_get_last(pf->cache);
        if (node == NULL) {

I think you should have kept the assert, not the if - look at the loop condition.

---

common/src/protected_files/protected_files.c line 953 at r1 (raw file):

    const unsigned char* data_to_write = (const unsigned char*)ptr;

    // the first 3KB of user data is written in the metadata node's encrypted part

Suggestion:

// the first MD_USER_DATA_SIZE bytes of user data is written in the metadata node's encrypted part

---

common/src/protected_files/protected_files.c line 1040 at r1 (raw file):

    unsigned char* out_buffer = (unsigned char*)ptr;

    // the first 3KB of user data is read from the metadata node's encrypted part

ditto

[dimakuv]

Reviewable status: 2 of 4 files reviewed, 7 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 @mkow)

---

common/src/protected_files/protected_files.c line 188 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I preferred the old name, "generating a key" usually means some randomness. Maybe this way?

Done.

---

common/src/protected_files/protected_files.c line 223 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

crypto slot of MHT node

What do you mean by that?

Please see the first diagram here: https://gramine.readthedocs.io/en/dimakuv-docs-encrypted-files/devel/encfiles.html#representation-on-host-storage-and-in-sgx-enclave-memory

Each MHT node has 96 "crypto slots", for each of the corresponding data nodes. Each "crypto slot" contains two values: 16-byte key and 16-byte MAC.

So my comment says that when the data node is being encrypted, the crypto slot (here referred via the variable gcm_crypto_data) is filled with two values: the code above fills the key, and the encryption operation here fills the MAC.

How do you want me to reformulate? Or maybe change some names (I'll rename some variables/fields in a follow up PR.)

---

common/src/protected_files/protected_files.c line 415 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you do that gluing of "meta" and "data" also in all other places in PF code?

Done.

---

common/src/protected_files/protected_files.c line 421 at r1 (raw file):

    }

    // encrypt metadata part-to-be-encrypted, also updates the MAC in metadata plaintext header

Done.

---

common/src/protected_files/protected_files.c line 550 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think you should have kept the assert, not the if - look at the loop condition.

Yeah, I'm just confused by this pf->last_error thing. I still don't understand what exactly it's used for.

But I agree, this particular case must have an assert, not an IF condition -- the cache cannot be empty if its size is larger than MAX_NODES_IN_CACHE constant.

---

common/src/protected_files/protected_files.c line 953 at r1 (raw file):

    const unsigned char* data_to_write = (const unsigned char*)ptr;

    // the first 3KB of user data is written in the metadata node's encrypted part

Done.

---

common/src/protected_files/protected_files.c line 1040 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

[mkow]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all 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), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

common/src/protected_files/protected_files.c line 223 at r1 (raw file):
But the link you pasted doesn't mention "crypto slots"? This name sounds very confusing to me, both "crypto" (are they other types of slots here? non-crypto slots?) and "slot" (do you mean an array item by it? if not, then what?).

Each MHT node has 96 "crypto slots", for each of the corresponding data nodes. Each "crypto slot" contains two values: 16-byte key and 16-byte MAC.

The diagram you linked to lists 96+32 such entries, not 96? One type is "data" and another is "mht", I'm still not sure which is "crypto".

How do you want me to reformulate? Or maybe change some names (I'll rename some variables/fields in a follow up PR.)

Depending what exactly you mean, but I guess it's just "corresponding {entry,array item,field} of MHT node".

[dimakuv]

Reviewable status: 3 of 4 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), "fixup! " found in commit messages' one-liners (waiting on @mkow)

---

common/src/protected_files/protected_files.c line 223 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But the link you pasted doesn't mention "crypto slots"? This name sounds very confusing to me, both "crypto" (are they other types of slots here? non-crypto slots?) and "slot" (do you mean an array item by it? if not, then what?).

Each MHT node has 96 "crypto slots", for each of the corresponding data nodes. Each "crypto slot" contains two values: 16-byte key and 16-byte MAC.

The diagram you linked to lists 96+32 such entries, not 96? One type is "data" and another is "mht", I'm still not sure which is "crypto".

How do you want me to reformulate? Or maybe change some names (I'll rename some variables/fields in a follow up PR.)

Depending what exactly you mean, but I guess it's just "corresponding {entry,array item,field} of MHT node".

Done.

Yes, "crypto slots" was my invention, but I agree that we should just go with a simple "array item".

[mkow]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners

[kailun-qin]

Reviewed 2 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[mkow]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved