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

Unmapping protected files does not work properly #231

Closed
pwmarcz opened this issue Nov 17, 2021 · 3 comments
Closed

Unmapping protected files does not work properly #231

pwmarcz opened this issue Nov 17, 2021 · 3 comments

Comments

@pwmarcz
Copy link
Contributor

pwmarcz commented Nov 17, 2021

Description of the problem

Gramine allows to map protected files, but unmapping the memory has no effect. As a result, closing the file can trigger saving garbage data to the file.

Found in #228 where copy_mmap_* tests trigger AddressSanitizer on file close.

Steps to reproduce

  1. Map a protected file using mmap
  2. Create another (anonymous) mapping over the same memory.
  3. Modify the underlying memory.
  4. Close the file.

On file close, Gramine read the underlying memory and save the data to the file, even though the file was not supposed to be mapped anymore.

Example code

The following example has been adapted from mmap_fixed.c LibOS regression test; you can overwrite that file to run it.

gramine-direct (and native Linux) print AAAA, gramine-sgx prints BBBB.

#define _GNU_SOURCE
#include <err.h>
#include <stdio.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

int main(void) {
    FILE* fp = fopen("tmp/pf/testfile", "w+");
    if (!fp)
        err(1, "fopen");

    if (ftruncate(fileno(fp), 1024))
        err(1, "ftruncate");

    char* a = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED, fileno(fp), 0);
    if (a == MAP_FAILED)
        err(1, "mmap");

    strcpy(a, "AAAA");

    a = mmap(a, 4096, PROT_READ | PROT_WRITE, MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    if (a == MAP_FAILED)
        err(1, "mmap");

    strcpy(a, "BBBB");

    if (fsync(fileno(fp)))
        err(1, "fsync");

    if (fclose(fp))
        err(1, "fclose");

    fp = fopen("tmp/pf/testfile", "r");
    if (!fp)
        err(1, "fopen");

    char buf[5];
    fread(buf, 5, 1, fp);
    printf("data: %.*s\n", 5, buf);

    if(fclose(fp))
        err(1, "fclose");

    return 0;
}

Additional information

LibOS should notice when a user tries to remove a mapping, and explicitly ask PAL to unmap the file. This doesn't happen at all. There's a DkStreamUnmap PAL function that removes the protected-file mappings, but it's not used anywhere.

Note that a proper fix should also handle unmap/remap of overlapping area (which would remove only part of the protected file mapping). The current implementation of DkStreamUnmap / flush_pf_maps does not do that.

@boryspoplawski
Copy link
Contributor

boryspoplawski commented Nov 17, 2021

I can quickly fix LibOS part, but from what you described we need to fix PAL part (especially PF part) first? If we unmmap part of the file, probably hell will break loose...

@dimakuv
Copy link

dimakuv commented Nov 17, 2021

I think the PAL part should be reasonably simple to fix for Protected Files. IIRC, PF contents are already maintained at page granularity, so unmapping a part of the file is a matter of flushing only that page range and splitting the "PF mmap" metadata object into two.

@pwmarcz
Copy link
Contributor Author

pwmarcz commented May 11, 2022

This is now fixed after the protected files rewrite: when mapping and unmapping memory, we use LibOS VMA subsystem as the source of truth, so we no longer incorrectly flush the anonymous mapping to a file.

I ran the example code to confirm (replacing tmp/pf/ with tmp_enc/).

@pwmarcz pwmarcz closed this as completed May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants