[LibOS,PAL] Add flexible device-specific IOCTL support #671

dimakuv · 2022-06-21T17:27:32Z

Description of the changes

The newly added ioctl() syscall emulation on device-backed file descriptors is pass-through. It is insecure by itself since the emulation only passes the arguments to and from the untrusted memory. It is the responsibility of the app developer to correctly use ioctls, with security implications in mind.

On the Linux-SGX PAL, a set of IOCTL requests must be explicitly allowed in the manifest via the new option sgx.allowed_ioctls.[id].request. Also, the allowed IOCTLs' arguments (typically pointers to complex nested objects) must be explicitly described in the manifest via the new options sgx.ioctl_structs.[id] and a corresponding reference sgx.allowed_ioctls.[id].struct; see docs for explanation of the IOCTL struct format.

Companion PR: gramineproject/device-testing-tools#4 (MERGED)

Next steps in future PRs:

Add caching of memory for regions and sub-regions (to avoid malloc/free all the time) -- probably useless.
Add string hashes, for quicker comparisons.
Add onlyif syntax. (Done in [common] Add onlyif syntax to flexible IOCTLs #1482.)
Add caching of sub-regions.
Add caching of IOCTL TOML definitions.

If reviewers think this is still too much functionality for the first commit, then I can strip off:

Pointer-arrays with size > 1 (but I already have a test for it!).

How to test this PR?

This PR adds a new LibOS test device_ioctl that tests the flexible IOCTL logic against Gramine dummy device /dev/gramine_test_dev. This device is found in companion repo gramineproject/device-testing-tools (https://github.com/gramineproject/device-testing-tools/tree/master/gramine-device-testing-module).

TODO:

Add a 22.04 bare-metal CI pipeline:
- Pass arbitrary Linux kernel command-line args in run.sh device-testing-tools#8
- [CI] Add a VM-based Jenkins pipeline #1032

This change is

dimakuv

Reviewable status: 0 of 20 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: Intel)

a discussion (no related file):
FYI: Rebased this PR to the latest master (to accommodate dir/file renames).

dimakuv

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):
@woju We are ready to start reviewing this PR. For this, we need to enable at least one CI machine with bare-metal Ubuntu 22.04 as the host OS (no VMs). Can we work on this?

boryspoplawski

Reviewed 2 of 18 files at r2, all commit messages.
Reviewable status: 2 of 22 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (1 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)

a discussion (no related file):
You use ioctl(),IOCTL and ioctl, would be good to unify.

-- commits line 2 at r3:

Suggestion:

PAL

-- commits line 8 at r3:
I would explain more precisely what you mean by "insecure". I would also mention that untrusted host can change arguments as it wishes, no matter how you use this feature.

Documentation/manifest-syntax.rst line 773 at r3 (raw file):

allows to explicitly allow a set of IOCTLs on devices (devices must be
explicitly mounted via ``fs.mounts`` manifest syntax). Only IOCTLs with the
``request`` argument found among the manifest-listed IOCTLs are allowed to

Maybe request code? Not sure which one descries it better.

Code quote:

``request`` argument

Documentation/manifest-syntax.rst line 778 at r3 (raw file):

Available IOCTL structs are described via ``sgx.ioctl_structs``. Each IOCTL
struct describes the memory layout of the ``arg`` argument (typically a pointer

What is arg argument? 3rd argument to ioctl syscall? Please be more precise here

Documentation/manifest-syntax.rst line 781 at r3 (raw file):

to a complex nested object passed to the device). Description of the memory
layout is required for a deep copy of the argument. The memory layout is
described using the TOML syntax of inline arrays (for each new separate memory

What is a "memory (sub-)region"?

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

  named ``strlen``, whereas ``size = 16`` denotes a sub-region of size 16B. Note
  that for ``ptr`` sub-regions, the ``size`` field has a different meaning: it
  denotes the number of adjacent memory regions (in other words, it denotes the

Ugh, can't we use size uniformly, i.e. in case of arrays just hole the size of the array, not length?

Documentation/manifest-syntax.rst line 806 at r3 (raw file):

- ``adjust`` is an optional integer adjustment for ``size``. It is 0 bytes by
  default. This field must be a constant (possibly negative) integer. For
  example, ``adjust = -8`` and ``size = 12`` results in a total size of 4B.

Is this field always in bytes or is it also affected by unit?

Documentation/manifest-syntax.rst line 819 at r3 (raw file):

  the implicit size of 8B, and the pointer value is always rewired to the memory
  region in untrusted memory (containing a copied-out nested memory region). If
  ``ptr`` is specified together with ``size``, it describes not just a pointer

Why special case it? Isn't in this context a pointer the same as pointer to an array of length == 1 (which is the default)?

Documentation/manifest-syntax.rst line 824 at r3 (raw file):

  layout.

Consider this simple example::

Please add some C code with some struct(s) definitions here, that would match the example below.
I assume it's something like:

struct st1 {
    struct st2* ptr;
};

struct st2 {
    char buf[0x1000];
} aligned(0x1000);

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Consider this simple example::

    sgx.ioctl_structs.st1 = [ { ptr=[ {name="nested_region", align=4096, size=4096, type="out"} ] } ]

Could you split this into multiple lines? I think it would be easier to read, e.g.:

sgx.ioctl_structs.st1 = [
    {
        ptr = [
            {
                name="nested_region",
                align=4096,
                size=4096,
                type="out"
            },
        ]
    },
]

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Consider this simple example::

    sgx.ioctl_structs.st1 = [ { ptr=[ {name="nested_region", align=4096, size=4096, type="out"} ] } ]

Why ptr is an TOML array of length == 1, not the dict directly? From the description above I understood this array must always have only one element (or be a string not an array - a name of another thingy).

Documentation/manifest-syntax.rst line 852 at r3 (raw file):

    sgx.allowed_ioctls.io3.request = 0x43218765  # this IOCTL's arg is passed as-is
    sgx.allowed_ioctls.io3.struct = "st2"

Wouldn't it be better to have empty name, i.e. "" mean no struct? Or even just omit the struct key of this entry? I find this empty array weird and unnecessarily complex.

libos/src/sys/libos_ioctl.c line 136 at r3 (raw file):

        }
        default: {
            lock(&g_dcache_lock);

Hmm, shouldn't this check happen before the switch?

dimakuv

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

a discussion (no related file):
This PR depends on #995. Otherwise Jenkins fails (see the explanation here: #990)

-- commits line 2 at r3:
Will do during the final rebase.

-- commits line 8 at r3:

Previously, boryspoplawski (Borys Popławski) wrote…

I would explain more precisely what you mean by "insecure". I would also mention that untrusted host can change arguments as it wishes, no matter how you use this feature.

What about this text:

  The newly added `ioctl()` syscall emulation on device-backed file
  descriptors is pass-through. It is insecure by itself since the
  emulation only passes the arguments to and from the untrusted memory:
    - ioctl arguments are passed as-is from the app to the untrusted host,
      without encryption; this may lead to leaks of secret data;
    - untrusted host can change ioctl arguments as it wishes when passing
      them from Gramine to the device and back.

  It is the responsibility of the app developer to correctly use ioctls,
  with security implications in mind. In particular, ioctl arguments should
  be encrypted or integrity-protected with a key pre-shared between
  Gramine and the device.

dimakuv

Reviewable status: 0 of 22 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@woju We are ready to start reviewing this PR. For this, we need to enable at least one CI machine with bare-metal Ubuntu 22.04 as the host OS (no VMs). Can we work on this?

@woju Ping.

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

You use ioctl(),IOCTL and ioctl, would be good to unify.

Done. I only found the occurences in log_error() cases -- there I replaced ioctl to IOCTL.

In all other instances, I seem to correctly use IOCTL as the name of the feature. And sometimes I use ioctl() when I want to highlight the system call itself (not just the feature of IOCTLs).

Documentation/manifest-syntax.rst line 773 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Maybe request code? Not sure which one descries it better.

I just followed man ioctl, which calls the arg as request. So to me, request is sensible.

Documentation/manifest-syntax.rst line 778 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What is arg argument? 3rd argument to ioctl syscall? Please be more precise here

Done.

Documentation/manifest-syntax.rst line 781 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What is a "memory (sub-)region"?

Done.

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ugh, can't we use size uniformly, i.e. in case of arrays just hole the size of the array, not length?

This is very hard to calculate and very hard to reason about. Also, if you've seen some IOCTL usages, it is almost always the case that arrays are specified as "length of the array" and never as "total size of the array". I can show some DRM (Direct Rendering Manager) examples to you, if you want to see some examples.

In other words, I'm mimicking how applications typically use IOCTL structs.

But I agree that I'm overloading this key size. If you want to, I can introduce another key count which will only be meaningful for ptr. But then it will get a bit complicated:

ptr may be used with count (by default count=1)
count without a ptr must be a syntax error
size with a ptr must be a syntax error

Would this proposal be better than overloading size for pointers and non-pointers?

Maybe I should even give a name to this key like array_count.

Documentation/manifest-syntax.rst line 806 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Is this field always in bytes or is it also affected by unit?

Done. It is always in bytes.

But if it's complicated for understanding, I can make it be affected by unit.

Documentation/manifest-syntax.rst line 819 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why special case it? Isn't in this context a pointer the same as pointer to an array of length == 1 (which is the default)?

Yes, that's what I do internally in the code.

I just thought that for normal users, it is hard to understand the concept of pointer == array[1]. Even C docs are fuzzy about this (or well, what people write on the internet about C arrays vs pointers). So the current text seemed more approripate to me than some strict statement like "ptr points to an array of memory regions, with a length of 1 by default".

I added an explainer, does this help?

Documentation/manifest-syntax.rst line 824 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add some C code with some struct(s) definitions here, that would match the example below.
I assume it's something like:
struct st1 {
    struct st2* ptr;
};

struct st2 {
    char buf[0x1000];
} aligned(0x1000);

Done.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Could you split this into multiple lines? I think it would be easier to read, e.g.:

sgx.ioctl_structs.st1 = [
    {
        ptr = [
            {
                name="nested_region",
                align=4096,
                size=4096,
                type="out"
            },
        ]
    },
]

Done.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why ptr is an TOML array of length == 1, not the dict directly? From the description above I understood this array must always have only one element (or be a string not an array - a name of another thingy).

No, your understanding is not correct. I updated the example to make it more pronounced.

The ptr points to a memory region, and this memory region may contain not one but several sub-regions. So we must first specify that we point to a memory region (via TOML array syntax [ ]), then we must specify each sub-region one by one (via TOML dict syntax { }).

Documentation/manifest-syntax.rst line 852 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wouldn't it be better to have empty name, i.e. "" mean no struct? Or even just omit the struct key of this entry? I find this empty array weird and unnecessarily complex.

Done.

libos/src/sys/libos_ioctl.c line 136 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Hmm, shouldn't this check happen before the switch?

Done.

boryspoplawski

Reviewed 2 of 4 files at r4, all commit messages.
Reviewable status: 2 of 22 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 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)

-- commits line 8 at r3:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What about this text:

  The newly added `ioctl()` syscall emulation on device-backed file
  descriptors is pass-through. It is insecure by itself since the
  emulation only passes the arguments to and from the untrusted memory:
    - ioctl arguments are passed as-is from the app to the untrusted host,
      without encryption; this may lead to leaks of secret data;
    - untrusted host can change ioctl arguments as it wishes when passing
      them from Gramine to the device and back.

  It is the responsibility of the app developer to correctly use ioctls,
  with security implications in mind. In particular, ioctl arguments should
  be encrypted or integrity-protected with a key pre-shared between
  Gramine and the device.

LGTM
The last para doesn't seem to be needed imo (it's commit message not docs and previous para already describes what are the risks), but it also can stay

Documentation/manifest-syntax.rst line 773 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I just followed man ioctl, which calls the arg as request. So to me, request is sensible.

And kernel uses cmd as argument name and command number, request number, code and many others to refer to it.
To me request sounds like the whole operation i.e. code + argument issued on a specific device.

Documentation/manifest-syntax.rst line 778 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

I would remove argp name, it lacks the context here (you seem to be following man, but it has the function signature, so it makes sense to use such names there; here it does not). IMO just Each IOCTL struct describes the memory layout of the third argument to the ``ioctl`` system call (typically...

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is very hard to calculate and very hard to reason about. Also, if you've seen some IOCTL usages, it is almost always the case that arrays are specified as "length of the array" and never as "total size of the array". I can show some DRM (Direct Rendering Manager) examples to you, if you want to see some examples.

In other words, I'm mimicking how applications typically use IOCTL structs.

But I agree that I'm overloading this key size. If you want to, I can introduce another key count which will only be meaningful for ptr. But then it will get a bit complicated:

ptr may be used with count (by default count=1)

count without a ptr must be a syntax error

size with a ptr must be a syntax error

Would this proposal be better than overloading size for pointers and non-pointers?

Maybe I should even give a name to this key like array_count.

IMO there's no good option here and we have to live with some inconsistencies. To me the only sensible options are:

size - has the problem of being non-intuitive for arrays,
length - technically with unit=1B, length is correct description of size, but it's counter intuitive to provide size of objects in length field

Documentation/manifest-syntax.rst line 819 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, that's what I do internally in the code.

I just thought that for normal users, it is hard to understand the concept of pointer == array[1]. Even C docs are fuzzy about this (or well, what people write on the internet about C arrays vs pointers). So the current text seemed more approripate to me than some strict statement like "ptr points to an array of memory regions, with a length of 1 by default".

I added an explainer, does this help?

The differences in C are between char* and char[]. Here you always have char* and just manipulate the length. I guess only not just a pointer is confusing to me, but with additional explanation it's at least clear after reading the whole paragraph, so I'm unblocking.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, your understanding is not correct. I updated the example to make it more pronounced.

The ptr points to a memory region, and this memory region may contain not one but several sub-regions. So we must first specify that we point to a memory region (via TOML array syntax [ ]), then we must specify each sub-region one by one (via TOML dict syntax { }).

Ok I got it. I understood it correctly that ptr is just one "thing", what I've missed is that a "memory region" is described as a TOML array ([ ]).

So a quick summary:

each struct is described by an array of items,
each item is described by a dict and corresponds to a element n C struct declaration list,
each item/dict has keys as described above (size, name, ...),
if an item is actually a pointer and memory that it points to needs to be copied, ptr = key is used; this key has either name of another struct or the struct definition inlined.

Is that correct?
So what you call "memory region" corresponds to a whole struct description and "sub-region" to what I've called an "item" above? Or actually "sub-region" could theoretically be multiple "items" as long as they are consecutive and have matching in-out params

Actually, wouldn't it be easier to just call everything an object? I.e. use object or just struct/structure in place of all memory regions and actually not use memory sub-region at all? What would be downsides of this?

Documentation/manifest-syntax.rst line 817 at r4 (raw file):

  negative) integer. For example, ``size = 6``, ``unit = 2`` and ``adjust = -8``
  results in a total size of 4B.
- ``type = ["none" | "out" | "in" | "inout"]`` is an optional direction of copy

type doesn't make sense to me in this context. Maybe direction or io or rw (but the last would probably require values to be changed to read/write)

Documentation/manifest-syntax.rst line 817 at r4 (raw file):

  negative) integer. For example, ``size = 6``, ``unit = 2`` and ``adjust = -8``
  results in a total size of 4B.
- ``type = ["none" | "out" | "in" | "inout"]`` is an optional direction of copy

I don't like this [ ] syntax here, it suggest a TOML array, whereas it's just one of the listed strings

Documentation/manifest-syntax.rst line 824 at r4 (raw file):

  specified for this sub-region (pointer sub-regions contain the pointer value
  which will be unconditionally rewired to point to untrusted memory).
- ``ptr = [ another memory region ]`` or ``ptr = "another-memory-region"``

[ ] suggest that there's array of "memory regions", while it's an inlined one (it's the "memory region" itself, that's a TOML array)

Suggestion:

`ptr = inlined-memory-region

Documentation/manifest-syntax.rst line 843 at r4 (raw file):

    struct st2 {
        size_t buf_size;
        char buf[0x1000];

This makes no sense, if we have buf_size, then this shouldn't be a static buffer.

Documentation/manifest-syntax.rst line 872 at r4 (raw file):

sub-region of size 4KB and that must be 4KB-aligned. This nested sub-region is
copied out of the enclave (but only ``buf_size`` bytes of the buffer are copied
out, to prevent memory leaks). The pointer value of the first memory region is

This new description doesn't make sense to me, but probably because this struct doesn't make sense (buffer of static size + dynamic size passed in addition)

Documentation/manifest-syntax.rst line 885 at r4 (raw file):

    sgx.allowed_ioctls.io2.struct = "st1"

If the IOCTL's third argument is simply an integer (or unused at all), then the

I guess it doesn't have to be an integer (in C sense)

Suggestion:

should be passed directly as-is

libos/src/sys/libos_ioctl.c line 136 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Not the read of is_host_dev, that doesn't matter at all. I mean the whole check - after all passthrough cmd can have any value handled above.

dimakuv

Reviewable status: 0 of 22 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

-- commits line 8 at r3:

Previously, boryspoplawski (Borys Popławski) wrote…

LGTM
The last para doesn't seem to be needed imo (it's commit message not docs and previous para already describes what are the risks), but it also can stay

Ok, I will remove the last paragraph. Moved to documentation (aparently I forgot to add notes about security in the docs).

-- commits line 11 at r4:
I'll need to change to request_code

Documentation/manifest-syntax.rst line 773 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

And kernel uses cmd as argument name and command number, request number, code and many others to refer to it.
To me request sounds like the whole operation i.e. code + argument issued on a specific device.

Done, switched to request_code

Documentation/manifest-syntax.rst line 778 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would remove argp name, it lacks the context here (you seem to be following man, but it has the function signature, so it makes sense to use such names there; here it does not). IMO just Each IOCTL struct describes the memory layout of the third argument to the ``ioctl`` system call (typically...

Done.

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

IMO there's no good option here and we have to live with some inconsistencies. To me the only sensible options are:

size - has the problem of being non-intuitive for arrays,

length - technically with unit=1B, length is correct description of size, but it's counter intuitive to provide size of objects in length field

Let me keep it as-is for now and wait for others' opinions (if any)

Documentation/manifest-syntax.rst line 819 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

The differences in C are between char* and char[]. Here you always have char* and just manipulate the length. I guess only not just a pointer is confusing to me, but with additional explanation it's at least clear after reading the whole paragraph, so I'm unblocking.

Done. I removed this not only a pointer part.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Or actually "sub-region" could theoretically be multiple "items" as long as they are consecutive and have matching in-out params

This is exactly correct. In fact, that's how I declared many DRM IOCTLs in my tests.

For example, consider this DRM IOCTL: https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm.h#L96. It can be described as a single sub-region, with size = 2*4. There is no benefit in splitting into four sub-regions.

On the other hand, we have DRM IOCTLs like this: https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/drm.h#L114. it contains the field padding, which must have direction = "none", to avoid any memory leaks. So here you have to create three sub-regions: one for consequtive next + prev + in_use, one for padding and one for age.

Actually, wouldn't it be easier to just call everything an object?

First, "object" is an overloaded term. Second, I wanted to make it explicit that we're taking about "regions of enclave memory". Third, I don't think that my example above (with drm_tex_region that has a field padding) will make sense with the word "object" (padding is an object? doesn't sound right).

Documentation/manifest-syntax.rst line 817 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I don't like this [ ] syntax here, it suggest a TOML array, whereas it's just one of the listed strings

We use this syntax in all places in this file (i.e., to list possible values for all other manifest options). But I agree, it's confusing. Let me change it here.

What should we do about the other manifest options? Should we fix them as well in a separate PR?

Documentation/manifest-syntax.rst line 817 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

type doesn't make sense to me in this context. Maybe direction or io or rw (but the last would probably require values to be changed to read/write)

Done. Chose direction.

Documentation/manifest-syntax.rst line 824 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

[ ] suggest that there's array of "memory regions", while it's an inlined one (it's the "memory region" itself, that's a TOML array)

Done.

Documentation/manifest-syntax.rst line 843 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This makes no sense, if we have buf_size, then this shouldn't be a static buffer.

Done. I wanted to have a contrived example that highlights many keywords, but I guess it's just confusing.

I now have a very simple and typical C struct.

Documentation/manifest-syntax.rst line 872 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This new description doesn't make sense to me, but probably because this struct doesn't make sense (buffer of static size + dynamic size passed in addition)

Done. Rewrote the description.

Documentation/manifest-syntax.rst line 885 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I guess it doesn't have to be an integer (in C sense)

Done.

libos/src/sys/libos_ioctl.c line 136 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Not the read of is_host_dev, that doesn't matter at all. I mean the whole check - after all passthrough cmd can have any value handled above.

Done.

boryspoplawski

Reviewed 2 of 4 files at r5, all commit messages.
Reviewable status: 2 of 22 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

There is no benefit in splitting into four sub-regions.

I would say otherwise, that there is no much gain in joining them into one. I think most people will use some kind of script to translate the structs into this description (or maybe even we should provide one in the future) and even doing this manually isn't much of a problem.

First, "object" is an overloaded term.

True, but this case is one of the less controversial cases, e.g. struct X* some_object = malloc(sizeof(*some_object)));

Second, I wanted to make it explicit that we're taking about "regions of enclave memory".

To me whole enclave memory is a "region". Idk, I find the current naming non-intuitive, but let's hear from others. cc @mkow

(padding is an object? doesn't sound right

It's not really padding, it's an object that's named padding, because that's the role it serves, but from C points of view it's not different from in_use field/object.

Documentation/manifest-syntax.rst line 817 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We use this syntax in all places in this file (i.e., to list possible values for all other manifest options). But I agree, it's confusing. Let me change it here.

What should we do about the other manifest options? Should we fix them as well in a separate PR?

I didn't look at them, but if they are confusing (i.e. it's not clear that they are not talking about TOML ararys), then I would change them. Maybe we should use <abc> or something else? Maybe just create an issue or discuss this on the next call?

boryspoplawski

Reviewed 3 of 20 files at r1, 12 of 18 files at r2, 2 of 2 files at r3, 1 of 4 files at r5.
Reviewable status: 20 of 22 files reviewed, 35 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

libos/test/regression/device_ioctl.c line 1 at r5 (raw file):

#include <alloca.h>

I don't see alloca being used

libos/test/regression/device_ioctl.c line 19 at r5 (raw file):

#define STRING_IOCTL_REPLACED "He$$0 w0r$d via i0ct$s\n"

struct gramine_test_dev_ioctl_write {

We shouldn't duplicate these struct and defines. Since we require the device to be installed, we can also require the headers.

libos/test/regression/device_ioctl.c line 65 at r5 (raw file):

                                                 struct gramine_test_dev_ioctl_replace_list)

int main(int argc, char* argv[]) {

Suggestion:

int main(void)

libos/test/regression/device_ioctl.c line 70 at r5 (raw file):

    char buf[64];

    int devfd = open("/dev/gramine_test_dev", O_RDWR);

You can reduce a lot of boilerplate from this file using CHECK macro

libos/test/regression/device_ioctl.c line 76 at r5 (raw file):

    /* test 1 -- use write() and read() syscalls */
    bytes = posix_fd_write(devfd, STRING_READWRITE, sizeof(STRING_READWRITE));
    if (bytes < 0)

Suggestion:

bytes != sizeof(STRING_READWRITE)

libos/test/regression/device_ioctl.c line 84 at r5 (raw file):

    if (offset < 0)
        err(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_REWIND)");
    if (offset > 0)

I would just change this and remove the check above (this error message prints the return value anyway)

Suggestion:

if (offset != 0)

libos/test/regression/device_ioctl.c line 90 at r5 (raw file):

    memset(&buf, 0, sizeof(buf));
    bytes = posix_fd_read(devfd, buf, sizeof(buf) - 1);
    if (bytes < 0)

Suggestion:

bytes != sizeof(STRING_READWRITE)

libos/test/regression/device_ioctl.c line 108 at r5 (raw file):

    if (ret < 0)
        err(1, "/dev/gramine_test_dev ioctl(GRAMINE_TEST_DEV_IOCTL_CLEAR)");

Maybe also use GRAMINE_TEST_DEV_IOCTL_GETSIZE here to confirm it returns 0 now?

libos/test/regression/device_ioctl.c line 150 at r5 (raw file):

                "(returned: %ld)", sizeof(STRING_IOCTL), devfd_size);

    /* test 3 -- use complex ioctl(GRAMINE_TEST_DEV_IOCTL_REPLACE_ARR) syscall */

Maybe with complex struct as an argument? The syscall itself is just the same (i.e. ioctl(fd, cmd, pointer))

libos/test/regression/device_ioctl.c line 156 at r5 (raw file):

    };
    struct gramine_test_dev_ioctl_replace_arr replace_arr = {
        .replacements_cnt = 2,

Suggestion:

ARRAY_LEN(replace_arr)

libos/test/regression/device_ioctl.c line 164 at r5 (raw file):

    memset(buf, 0, sizeof(buf));
    read_arg.off = 0;

Please reassign read_arg, at least to make sure copied is reset.

Suggestion:

read_arg = (struct gramine_test_dev_ioctl_read){
    .buf_size = sizeof(buf) - 1,
    .buf      = buf,
};

libos/test/regression/device_ioctl.c line 172 at r5 (raw file):

             STRING_IOCTL_REPLACED);

    /* test 4 -- use complex ioctl(GRAMINE_TEST_DEV_IOCTL_REPLACE_LIST) syscall */

ditto

libos/test/regression/device_ioctl.c line 187 at r5 (raw file):

    memset(buf, 0, sizeof(buf));
    read_arg.off = 0;

ditto

libos/test/regression/device_ioctl.manifest.template line 13 at r5 (raw file):

]

sgx.nonpie_binary = true

Why this?

libos/test/regression/device_ioctl.manifest.template line 23 at r5 (raw file):


# for IOCTLs without an argument (or with integer argument)
sgx.ioctl_structs.gramine_test_dev_ioctl_dummy = [ ]

Not needed anymore?

libos/test/regression/device_ioctl.manifest.template line 27 at r5 (raw file):

# three IOCTLs below test different "no struct needed" syntaxes of the `struct` key
sgx.allowed_ioctls.GRAMINE_TEST_DEV_IOCTL_REWIND.request_code = 0x8100
sgx.allowed_ioctls.GRAMINE_TEST_DEV_IOCTL_REWIND.struct = "gramine_test_dev_ioctl_dummy"

Is this still supported?

libos/test/regression/device_ioctl.manifest.template line 35 at r5 (raw file):


sgx.ioctl_structs.gramine_test_dev_ioctl_write = [
    { size=8, direction="out", name="buf_size" },     # buf_size

Please add spaces around = to match our conventions
ditto everywhere

libos/test/regression/device_ioctl.manifest.template line 38 at r5 (raw file):

    { ptr=[ {size="buf_size", direction="out"} ] },   # buf
    { size=8, direction="inout" },                    # off
    { adjust=-4, size=12, direction="in" },           # copied; adjust is just for testing

Uh, this is confusing, can't we have some other struct which would use this adjust feature?

libos/test/regression/device_ioctl.manifest.template line 57 at r5 (raw file):

    { size=8, direction="out", name="replacements_cnt" },  # replacements_cnt
    { size="replacements_cnt", ptr=[                       # replacements_arr
                                     { size=2, units=1, direction="out" },  # src, dst

Why is this needed?

Code quote:

units=1

pal/include/pal/pal.h line 874 at r5 (raw file):

 *                         or used as a pointer to a buffer that contains the data required to
 *                         perform the operation as well as the data returned by the operation. For
 *                         some PALs (e.g., Linux-SGX PAL), the manifest must describe the layout of

Suggestion:

(e.g. Linux-SGX),

pal/include/pal/pal.h line 883 at r5 (raw file):

 * This function corresponds to ioctl() in UNIX systems and DeviceIoControl() in Windows.
 */
int PalDeviceIoControl(PAL_HANDLE handle, uint32_t cmd, uint64_t arg, int* out_ret);

Hmm, shouldn't this be 32bit on 32bit archs?
unsigned long or uintptr_t should work, I would prefer the former, since we already have assumption it's equal to arch's natural size, but I'm not sure if we have it everywhere or only in LibOS.

Code quote:

uint64_t arg

pal/src/host/linux/pal_devices.c line 224 at r5 (raw file):

        return unix_to_pal_error(ret);

    *out_ret = ret;

Why not this (below)? Why make negative values special? Also why is PAL returning UNIX error codes (via out_ret)? What would this suppose to look like on non-Linux PAL?

Suggestion:

    *out_ret = DO_SYSCALL(ioctl, handle->dev.fd, cmd, arg);

pal/src/host/linux-sgx/enclave_ocalls.c line 1993 at r5 (raw file):

int ocall_ioctl(int fd, unsigned int cmd, unsigned long arg) {
    int retval;
    ms_ocall_ioctl_t* ms;

Will have to be renamed once you rebase to the current master (we dropped ms_ prefix)

pal/src/host/linux-sgx/enclave_ocalls.c line 1996 at r5 (raw file):

    void* old_ustack = sgx_prepare_ustack();
    ms = sgx_alloc_on_ustack_aligned(sizeof(*ms), alignof(*ms));

Suggestion:

ms_ocall_ioctl_t* ms =

pal/src/host/linux-sgx/enclave_ocalls.c line 2008 at r5 (raw file):

    do {
        retval = sgx_exitless_ocall(OCALL_IOCTL, ms);
    } while (retval == -EINTR);

ioctl shouldn't loop on EINTR.

pal/src/host/linux-sgx/enclave_ocalls.c line 2011 at r5 (raw file):

    if (retval < 0 && retval != -EBADF && retval != -EFAULT && retval != -EINVAL &&
            retval != -ENOTTY) {

Uhh, isn't it up to the driver to decide which error codes are valid? I.e. in general case all of them are.

pal/src/host/linux-sgx/host_ocalls.c line 734 at r5 (raw file):

static long sgx_ocall_ioctl(void* pms) {
    ms_ocall_ioctl_t* ms = (ms_ocall_ioctl_t*)pms;

Suggestion:

ms_ocall_ioctl_t* ms = pms;

pal/src/host/linux-sgx/host_ocalls.c line 736 at r5 (raw file):

    ms_ocall_ioctl_t* ms = (ms_ocall_ioctl_t*)pms;
    long ret = DO_SYSCALL(ioctl, ms->ms_fd, ms->ms_cmd, ms->ms_arg);
    return ret;

Suggestion:

    return DO_SYSCALL(ioctl, ms->ms_fd, ms->ms_cmd, ms->ms_arg);

dimakuv

Reviewable status: 10 of 22 files reviewed, 35 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

libos/test/regression/device_ioctl.c line 1 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I don't see alloca being used

Done.

libos/test/regression/device_ioctl.c line 19 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We shouldn't duplicate these struct and defines. Since we require the device to be installed, we can also require the headers.

Done. See also corresponding changes in gramineproject/device-testing-tools#3

libos/test/regression/device_ioctl.c line 65 at r5 (raw file):

                                                 struct gramine_test_dev_ioctl_replace_list)

int main(int argc, char* argv[]) {

Done.

libos/test/regression/device_ioctl.c line 70 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

You can reduce a lot of boilerplate from this file using CHECK macro

Done.

libos/test/regression/device_ioctl.c line 76 at r5 (raw file):

    /* test 1 -- use write() and read() syscalls */
    bytes = posix_fd_write(devfd, STRING_READWRITE, sizeof(STRING_READWRITE));
    if (bytes < 0)

Done.

libos/test/regression/device_ioctl.c line 84 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would just change this and remove the check above (this error message prints the return value anyway)

Done. Not just removed the check above but wrapped it into CHECK().

libos/test/regression/device_ioctl.c line 90 at r5 (raw file):

    memset(&buf, 0, sizeof(buf));
    bytes = posix_fd_read(devfd, buf, sizeof(buf) - 1);
    if (bytes < 0)

Done.

libos/test/regression/device_ioctl.c line 108 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Maybe also use GRAMINE_TEST_DEV_IOCTL_GETSIZE here to confirm it returns 0 now?

Done.

libos/test/regression/device_ioctl.c line 150 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Maybe with complex struct as an argument? The syscall itself is just the same (i.e. ioctl(fd, cmd, pointer))

Done.

libos/test/regression/device_ioctl.c line 156 at r5 (raw file):

    };
    struct gramine_test_dev_ioctl_replace_arr replace_arr = {
        .replacements_cnt = 2,

Done.

libos/test/regression/device_ioctl.c line 164 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please reassign read_arg, at least to make sure copied is reset.

Done.

I'm not sure what you want to do with copied. Do you just want it to be reset? Or do you also want to check this field after the IOCTL?

libos/test/regression/device_ioctl.c line 172 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

libos/test/regression/device_ioctl.c line 187 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

libos/test/regression/device_ioctl.manifest.template line 13 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this?

It's just set in all our regression tests. Not needed, but helpful in debugging (one base address less to change from debug session to debug session).

libos/test/regression/device_ioctl.manifest.template line 23 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Not needed anymore?

We still support this syntax. I see no downside in supporting this special case.

libos/test/regression/device_ioctl.manifest.template line 27 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Is this still supported?

Yes. Do you think it's only confusing, and I should remove this "empty-array" syntax completely?

libos/test/regression/device_ioctl.manifest.template line 35 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add spaces around = to match our conventions
ditto everywhere

Done.

libos/test/regression/device_ioctl.manifest.template line 38 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Uh, this is confusing, can't we have some other struct which would use this adjust feature?

Done, moved to gramine_test_dev_ioctl_replace_list. Though it's still a bit confusing (purely for testing). But I couldn't come up with a "natural" use for this adjust keyword in this example.

By the way, the typical use for adjust keyword is when there is an IOCTL struct that has a fixed-size header and an arbitrary-size data. In this case the data size is sometimes specified as "whole ioctl struct size - header size". That's why we need size = "ioctl_struct_size_field", adjust = -12 (where 12 is the fixed size of the header). I've encountered this case only in a couple DRM ioctls, so it's pretty rare and unconventional.

libos/test/regression/device_ioctl.manifest.template line 57 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why is this needed?

Done. Removed from here and put into another IOCTL + added a comment that it's purely for testing.

pal/include/pal/pal.h line 874 at r5 (raw file):

 *                         or used as a pointer to a buffer that contains the data required to
 *                         perform the operation as well as the data returned by the operation. For
 *                         some PALs (e.g., Linux-SGX PAL), the manifest must describe the layout of

Done.

pal/include/pal/pal.h line 883 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Hmm, shouldn't this be 32bit on 32bit archs?
unsigned long or uintptr_t should work, I would prefer the former, since we already have assumption it's equal to arch's natural size, but I'm not sure if we have it everywhere or only in LibOS.

Done, though I guess our codebase is absolutely not ready for 32-bit archs anyway.

pal/src/host/linux/pal_devices.c line 224 at r5 (raw file):

Why make negative values special?

Negative values from the host ioctl() are UNIX error codes, which we translate into PAL error codes.

At the same time, non-negative values are ioctl-specific, and we should return them as-is.

Also, we have a convention in our code that PAL functions return 0 on success, so I introduce *out_ret to return ioctl (non-negative) values.

Also why is PAL returning UNIX error codes (via out_ret)?

No, this is not possible. PAL never returns UNIX error codes. out_ret always returns non-negative values, because in the above lines we have a check for ret < 0 and return immediately there.

I guess you missed the fact that many IOCTLs return some this-ioctl-specific positive value, and not just zero.

pal/src/host/linux-sgx/enclave_ocalls.c line 1993 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Will have to be renamed once you rebase to the current master (we dropped ms_ prefix)

Yes. Blocked myself.

pal/src/host/linux-sgx/enclave_ocalls.c line 1996 at r5 (raw file):

    void* old_ustack = sgx_prepare_ustack();
    ms = sgx_alloc_on_ustack_aligned(sizeof(*ms), alignof(*ms));

Done.

pal/src/host/linux-sgx/enclave_ocalls.c line 2008 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ioctl shouldn't loop on EINTR.

Done.

pal/src/host/linux-sgx/enclave_ocalls.c line 2011 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Uhh, isn't it up to the driver to decide which error codes are valid? I.e. in general case all of them are.

Yeah, good point. These were collected from a subset of ioctls that I played with (mostly DRM ioctls). Ok, let's remove this check.

pal/src/host/linux-sgx/host_ocalls.c line 734 at r5 (raw file):

static long sgx_ocall_ioctl(void* pms) {
    ms_ocall_ioctl_t* ms = (ms_ocall_ioctl_t*)pms;

Done.

pal/src/host/linux-sgx/host_ocalls.c line 736 at r5 (raw file):

    ms_ocall_ioctl_t* ms = (ms_ocall_ioctl_t*)pms;
    long ret = DO_SYSCALL(ioctl, ms->ms_fd, ms->ms_cmd, ms->ms_arg);
    return ret;

Done.

boryspoplawski

Reviewed 10 of 11 files at r6.
Reviewable status: 19 of 22 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

libos/test/regression/device_ioctl.c line 164 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

I'm not sure what you want to do with copied. Do you just want it to be reset? Or do you also want to check this field after the IOCTL?

Sorry, I thought it was used to in some checks below, but seems it's not.

libos/test/regression/device_ioctl.manifest.template line 13 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It's just set in all our regression tests. Not needed, but helpful in debugging (one base address less to change from debug session to debug session).

It's set in mnifest.template in LibOS regression tests, because we have one template for multiple tests and some of them need this option. Here it's not needed.

libos/test/regression/device_ioctl.manifest.template line 27 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes. Do you think it's only confusing, and I should remove this "empty-array" syntax completely?

Yes, I don't see why anybody would want to use it (create some dummy structs), if they can simply write "" or even omit this field.

libos/test/regression/device_ioctl.manifest.template line 66 at r6 (raw file):


sgx.ioctl_structs.gramine_test_dev_ioctl_replace_list = [
    { size = 2, units = 1, direction = "out" },    # src, dst; `units` is just for testing

If it's for testing, then maybe do size = 1, units = 2, because units = 1 is the default

libos/test/regression/meson.build line 19 at r6 (raw file):

        'include_directories': include_directories(
            # for `gramine_test_dev_ioctl.h`
            '/usr/local/include/',

This path is included by default

pal/include/pal/pal.h line 874 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

comma still there

pal/include/pal/pal.h line 883 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done, though I guess our codebase is absolutely not ready for 32-bit archs anyway.

I wouldn't say absolutely, there would be issues definitely, especially with the old code, but it should be doable (though with much effort).

pal/src/host/linux/pal_devices.c line 224 at r5 (raw file):

I guess you missed the fact that many IOCTLs return some this-ioctl-specific positive value, and not just zero.

No, I was just confused why it returns device-specific return values via out_ret but error codes, which can also be device-specific, directly.

Generally this whole feature will make no sense on non-Linux PALs (because original ioctl requests come from a Linux app), so it's hard for me to reason about conventions here.

boryspoplawski

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 20 of 22 files reviewed, 16 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

libos/test/regression/device_ioctl.c line 1 at r7 (raw file):

#define _GNU_SOURCE /* for loff_t */

Actually, shouldn't we use off_t for user facing API (ioctls)? TBH I not sure what are the rules here, which type should be used where...

boryspoplawski

Reviewed 1 of 18 files at r2, 1 of 11 files at r6.
Reviewable status: all files reviewed, 67 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

pal/src/host/linux-sgx/pal_devices.c line 225 at r7 (raw file):

/*
 * Code below describes the deep-copy syntax in the TOML manifest used for copying complex nested

This read weirdly for me (deep-copy syntax?), maybe:

Suggestion:

Code below describes the TOML syntax of the manifest entries used for deep-copying complex nested

pal/src/host/linux-sgx/pal_devices.c line 228 at r7 (raw file):

 * objects out and inside the SGX enclave. This syntax is currently used for IOCTL emulation. This
 * syntax is generic enough to describe any memory layout for deep copy of IOCTL structs.
 *

I would add a note here, that "high level"/"syntax" description is in "manifest syntax" docs.

pal/src/host/linux-sgx/pal_devices.c line 236 at r7 (raw file):

 *
 *   alignas(128) struct root obj;
 *   ioctl(devfd, _IOWR(DEVICE_MAGIC, DEVICE_FUNC, struct root), &obj);

Just so you are able to refer it below

Suggestion:

DEV_IOCTL_NUMBER

pal/src/host/linux-sgx/pal_devices.c line 249 at r7 (raw file):

 * occurences of chars "x" and "y" in the Pascal string.
 *
 * The corresponding deep-copy syntax in TOML looks like this:

Suggestion:

The corresponding manifest entries describing this structs look like this:

pal/src/host/linux-sgx/pal_devices.c line 252 at r7 (raw file):

 *
 *   sgx.ioctl_structs.ROOT_FOR_DEVICE_FUNC = [
 *     { align = 128, ptr = [ {name="pascal-str-len", size=1, direction="out"},

Please add more spaces (especially around =).

pal/src/host/linux-sgx/pal_devices.c line 254 at r7 (raw file):

 *     { align = 128, ptr = [ {name="pascal-str-len", size=1, direction="out"},
 *                            {name="pascal-str", size="pascal-str-len", direction="out"} ] },
 *     { ptr = [ {name="c-str", size="c-str-len", direction="in"} ], size = 1 },

This looks wrong to me. Am I missing something?

Code quote:

size = 1

pal/src/host/linux-sgx/pal_devices.c line 255 at r7 (raw file):

 *                            {name="pascal-str", size="pascal-str-len", direction="out"} ] },
 *     { ptr = [ {name="c-str", size="c-str-len", direction="in"} ], size = 1 },
 *     { name = "c-str-len", size = 8, unit = 1, adjust = 0, direction = "inout" },

These make it only more confusing. This is not docs trying to describe all possible options, so I would just remove them

Code quote:

 unit = 1, adjust = 0,

pal/src/host/linux-sgx/pal_devices.c line 256 at r7 (raw file):

 *     { ptr = [ {name="c-str", size="c-str-len", direction="in"} ], size = 1 },
 *     { name = "c-str-len", size = 8, unit = 1, adjust = 0, direction = "inout" },
 *     { size = 2, direction = "in" }

Wait, why is this size = 2?

pal/src/host/linux-sgx/pal_devices.c line 257 at r7 (raw file):

 *     { name = "c-str-len", size = 8, unit = 1, adjust = 0, direction = "inout" },
 *     { size = 2, direction = "in" }
 *     { size = 2, direction = "in" }

ditto

pal/src/host/linux-sgx/pal_devices.c line 260 at r7 (raw file):

 *   ]
 *
 *   sgx.allowed_ioctls.DEVICE_FUNC.request_code = <DEVICE_MAGIC hex>

If you rename above then you could refer to it also here.
Note that in your example DEVICE_MAGIC is just some constant, which is used as argument to ioctl-number creating macros (like _IOW), so this is a misleading.

Suggestion:

DEV_IOCTL_NUMBER

pal/src/host/linux-sgx/pal_devices.c line 268 at r7 (raw file):

 *  2. Each sub-region of one memory region is represented as a TOML table (`{}`).
 *  3. Each sub-region may be a pointer (`ptr`) to another memory region. In this case, the value of
 *     `ptr` is a TOML-array representation of that other memory region. The `ptr` sub-region always

It can also be a name of another struct/memory region, right?

pal/src/host/linux-sgx/pal_devices.c line 269 at r7 (raw file):

 *  3. Each sub-region may be a pointer (`ptr`) to another memory region. In this case, the value of
 *     `ptr` is a TOML-array representation of that other memory region. The `ptr` sub-region always
 *     has size of 8B (assuming x86-64) and doesn't have an in/out direction.  The `size` field of

Actually, why is size hardcoded? 8B is a good default, but why it cannot be overwritten explicitly?

pal/src/host/linux-sgx/pal_devices.c line 270 at r7 (raw file):

 *     `ptr` is a TOML-array representation of that other memory region. The `ptr` sub-region always
 *     has size of 8B (assuming x86-64) and doesn't have an in/out direction.  The `size` field of
 *     the `ptr` sub-region has a different meaning than for non-pointer sub-regions: it is the

So, to continue the discussion from docs, this is a bit confusing. Maybe we should have size mean the size of the pointer and a new length parameter, which would describe length of the array?

pal/src/host/linux-sgx/pal_devices.c line 275 at r7 (raw file):

 *     can be flexible-size (like the two strings). In the latter case, the `size` field contains a
 *     name of a sub-region where the actual size is stored.
 *  5. Sub-regions that store the size of another sub-region must be 1, 2, 4, or 8 bytes in size.

Why this limitation? (<= 8 I would understand)
Don't you memcpy the size anyway?

pal/src/host/linux-sgx/pal_devices.c line 279 at r7 (raw file):

 *     sub-regions but may be omitted for all other kinds of sub-regions.
 *  7. Sub-regions may have one of the four directions: "out" to copy contents of the sub-region
 *     outside the enclave to untrusted memory, "in" to copy from untrusted memory to inside the

Suggestion:

out of

pal/src/host/linux-sgx/pal_devices.c line 279 at r7 (raw file):

 *     sub-regions but may be omitted for all other kinds of sub-regions.
 *  7. Sub-regions may have one of the four directions: "out" to copy contents of the sub-region
 *     outside the enclave to untrusted memory, "in" to copy from untrusted memory to inside the

Suggestion:

into

pal/src/host/linux-sgx/pal_devices.c line 282 at r7 (raw file):

 *     enclave, "inout" to copy in both directions, "none" to not copy at all (useful for e.g.
 *     padding). Note that pointer sub-regions do not have a direction (their values are
 *     unconditionally rewired so as to point to the copied-out region in untrusted memory).

Actually the pointer region can be an input argument, so it wouldn't be "copied-out"

pal/src/host/linux-sgx/pal_devices.c line 299 at r7 (raw file):

 *  |   |  uint64_t s2_len | |   SR3    |    |  |   uint64_t s2_len      |  |
 *  |   |                  | |          |    |  |                        |  |
 *  |   |  int8_t x, y     | |   SR4    |    |  |   int8_t x=0, y=0      |  |

?

Code quote:

x=0, y=0

pal/src/host/linux-sgx/pal_devices.c line 322 at r7 (raw file):

/* direction of copy: none (used for padding), out of enclave, inside enclave, both or a special
 * "pointer" sub-region; default is COPY_NONE_ENCLAVE */
enum mem_copy_direction {COPY_NONE_ENCLAVE, COPY_OUT_ENCLAVE, COPY_IN_ENCLAVE, COPY_INOUT_ENCLAVE,

Please split into separate lines.

pal/src/host/linux-sgx/pal_devices.c line 322 at r7 (raw file):

/* direction of copy: none (used for padding), out of enclave, inside enclave, both or a special
 * "pointer" sub-region; default is COPY_NONE_ENCLAVE */
enum mem_copy_direction {COPY_NONE_ENCLAVE, COPY_OUT_ENCLAVE, COPY_IN_ENCLAVE, COPY_INOUT_ENCLAVE,

This _ENCLAVE suffix makes little sense to me (I assume it means "copy out of the enclave", but not sure).
IMO better alternatives:

DIRECTION_NONE (my preference out of these),
COPY_NONE,
MEM_COPY_NONE.

pal/src/host/linux-sgx/pal_devices.c line 332 at r7 (raw file):

struct sub_region {
    enum mem_copy_direction direction; /* direction of copy during OCALL */

misaligned comment

pal/src/host/linux-sgx/pal_devices.c line 335 at r7 (raw file):

    char* name;               /* may be NULL for unnamed regions */
    uint64_t name_hash;       /* hash of "name" for fast string comparison */
    ssize_t align;            /* alignment of this sub-region */

Why signed?

pal/src/host/linux-sgx/pal_devices.c line 336 at r7 (raw file):

    uint64_t name_hash;       /* hash of "name" for fast string comparison */
    ssize_t align;            /* alignment of this sub-region */
    ssize_t size;             /* may be dynamically determined from another sub-region */

ditto

pal/src/host/linux-sgx/pal_devices.c line 339 at r7 (raw file):

    char* size_name;          /* needed if "size" sub region is defined after this sub region */
    uint64_t size_name_hash;  /* needed if "size" sub region is defined after this sub region */
    ssize_t unit;             /* total size in bytes is calculated as `size * unit + adjust` */

ditto

pal/src/host/linux-sgx/pal_devices.c line 343 at r7 (raw file):

    void* encl_addr;          /* base address of this sub region in enclave memory */
    void* untrusted_addr;     /* base address of the corresponding sub region in untrusted memory */
    toml_array_t* mem_ptr;    /* for pointers/arrays, specifies pointed-to mem region */

I'm surprised. Shouldn't this be struct mem_region*?
I've not yet read rest of the code, but from the description I would expect it

pal/src/host/linux-sgx/pal_devices.c line 346 at r7 (raw file):

};

static inline uint64_t hash(char* str) {

Suggestion:

static uint64_t hash(const char* str)

pal/src/host/linux-sgx/pal_devices.c line 351 at r7 (raw file):

    char c;
    while ((c = *str++))
        hash = ((hash << 5) + hash) + c;

Suggestion:

(hash << 5) + hash

pal/src/host/linux-sgx/pal_devices.c line 358 at r7 (raw file):

    if (!s1 || !s2 || s1_hash != s2_hash)
        return false;
    assert(s1_hash == s2_hash);

We literally check this in the line above...

pal/src/host/linux-sgx/pal_devices.c line 367 at r7 (raw file):

                              size_t* out_idx) {
    /* it is important to iterate in reverse order because there may be an array of mem regions
     * with same-named sub regions, and we want to find the latest sub region */

I don't understand this. Maybe it gets cleaner when I read the usage.

pal/src/host/linux-sgx/pal_devices.c line 374 at r7 (raw file):

            /* found corresponding sub region */
            if (sub_regions[idx].direction != COPY_PTR_ENCLAVE || !sub_regions[idx].mem_ptr) {
                /* sub region is not a valid pointer to a memory region */

Description above, and more importantly the name of this function do not imply this requirement...

pal/src/host/linux-sgx/pal_devices.c line 422 at r7 (raw file):

}

/* caller sets `sub_regions_cnt_ptr` to maximum number of sub_regions; this variable is updated to

Suggestion:

caller sets `sub_regions_cnt_ptr` to the length of `sub_regions` array

pal/src/host/linux-sgx/pal_devices.c line 424 at r7 (raw file):

/* caller sets `sub_regions_cnt_ptr` to maximum number of sub_regions; this variable is updated to
 * return the number of actually used sub_regions */
static int collect_sub_regions(toml_array_t* root_toml_array, void* root_encl_addr,

This function is quite big. Could it be split into multiple? I've not reviewed it yet, so maybe it makes no sense to do so.

pal/src/host/linux-sgx/pal_devices.c line 429 at r7 (raw file):

    assert(root_toml_array && toml_array_nelem(root_toml_array) > 0);
    assert(sub_regions && sub_regions_cnt_ptr);

I think this code works correctly without this assumption, I would remove it.

pal/src/host/linux-sgx/pal_devices.c line 737 at r7 (raw file):

    char* cur_untrusted_addr = untrusted_addr;
    for (size_t i = 0; i < sub_regions_cnt; i++) {
        if (sub_regions[i].size <= 0 || !sub_regions[i].encl_addr)

Why is it possible to have encl_addr == NULL and size > 0?

pal/src/host/linux-sgx/pal_devices.c line 747 at r7 (raw file):

        if (sub_regions[i].direction == COPY_OUT_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE) {

Suggestion:

        if (sub_regions[i].direction == COPY_OUT_ENCLAVE
                || sub_regions[i].direction == COPY_INOUT_ENCLAVE) {

pal/src/host/linux-sgx/pal_devices.c line 748 at r7 (raw file):

        if (sub_regions[i].direction == COPY_OUT_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE) {
            memcpy(cur_untrusted_addr, sub_regions[i].encl_addr, sub_regions[i].size);

Please use sgx_copy_from_enclave

pal/src/host/linux-sgx/pal_devices.c line 758 at r7 (raw file):

    for (size_t i = 0; i < sub_regions_cnt; i++) {
        if (sub_regions[i].size <= 0 || !sub_regions[i].encl_addr)

ditto

pal/src/host/linux-sgx/pal_devices.c line 766 at r7 (raw file):

            for (size_t j = 0; j < sub_regions_cnt; j++) {
                if (sub_regions[j].encl_addr == encl_ptr_value) {
                    *((void**)sub_regions[i].untrusted_addr) = sub_regions[j].untrusted_addr;

Please use sgx_copy_from_enclave

pal/src/host/linux-sgx/pal_devices.c line 776 at r7 (raw file):

static void copy_sub_regions_to_enclave(struct sub_region* sub_regions, size_t sub_regions_cnt) {
    for (size_t i = 0; i < sub_regions_cnt; i++) {
        if (sub_regions[i].size <= 0 || !sub_regions[i].encl_addr)

ditto

pal/src/host/linux-sgx/pal_devices.c line 780 at r7 (raw file):

        if (sub_regions[i].direction == COPY_IN_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE)

Suggestion:

        if (sub_regions[i].direction == COPY_IN_ENCLAVE
                || sub_regions[i].direction == COPY_INOUT_ENCLAVE)

pal/src/host/linux-sgx/pal_devices.c line 780 at r7 (raw file):

        if (sub_regions[i].direction == COPY_IN_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE)

I would also prefer { } since this is a multiline condition, but just personal preference (not blocking)

pal/src/host/linux-sgx/pal_devices.c line 781 at r7 (raw file):

        if (sub_regions[i].direction == COPY_IN_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE)
            memcpy(sub_regions[i].encl_addr, sub_regions[i].untrusted_addr, sub_regions[i].size);

sgx_copy_to_enclave

pal/src/host/linux-sgx/pal_devices.c line 802 at r7 (raw file):

        return -PAL_ERROR_NOTIMPLEMENTED;

    for (ssize_t idx = 0; idx < toml_allowed_ioctls_cnt; idx++) {

Suggestion:

for (size_t idx = 0; idx < (size_t)toml_allowed_ioctls_cnt; idx++) {

pal/src/host/linux-sgx/pal_devices.c line 807 at r7 (raw file):

        toml_table_t* toml_ioctl_table = toml_table_in(toml_allowed_ioctls, toml_allowed_ioctl_key);
        if (!toml_ioctl_table)

Is this even possible?

pal/src/host/linux-sgx/pal_devices.c line 816 at r7 (raw file):

        int64_t ioctl_request_code = 0x0;
        ret = toml_rtoi(toml_ioctl_request_code_raw, &ioctl_request_code);
        if (ret < 0 || ioctl_request_code == 0x0) {

From where comes the assumption that 0 is invalid ioctl request code?

pal/src/host/linux-sgx/pal_devices.c line 820 at r7 (raw file):

                      toml_allowed_ioctl_key);
            continue;
        }

Why not toml_int_in?

pal/src/host/linux-sgx/pal_devices.c line 822 at r7 (raw file):

        }

        if (ioctl_request_code == (int64_t)cmd) {

I think moving the whole inside of this if to a separate function would make it more readable.

pal/src/host/linux-sgx/pal_devices.c line 873 at r7 (raw file):

/*
 * Thread-local scratch space for IOCTL internal data:

Why do we even need this?
It's around 1MB of PAL internal memory per thread...
Also this code is O(#sub_regions^2) (or more, didn't review all of it), so I wouldn't expect allocations to worsen performance that much...

pal/src/host/linux-sgx/pal_devices.c line 874 at r7 (raw file):

/*
 * Thread-local scratch space for IOCTL internal data:
 *   1. Memregions array of size MAX_MEM_REGIONS +

This + is weird.

pal/src/host/linux-sgx/pal_devices.c line 886 at r7 (raw file):

    size_t total_size = MAX_MEM_REGIONS * sizeof(struct mem_region) +
                            MAX_SUB_REGIONS * sizeof(struct sub_region);

Suggestion:

    size_t total_size = MAX_MEM_REGIONS * sizeof(struct mem_region)
                        + MAX_SUB_REGIONS * sizeof(struct sub_region);

pal/src/host/linux-sgx/pal_devices.c line 925 at r7 (raw file):

    size_t sub_regions_cnt = MAX_SUB_REGIONS;
    struct sub_region* sub_regions = (struct sub_region*)pal_get_enclave_tcb()->ioctl_scratch_space +
                                     MAX_MEM_REGIONS * sizeof(struct mem_region);

This is wrong. You actually add MAX_MEM_REGIONS * sizeof(struct mem_region) * sizeof(struct sub_region).
Also we put dangling operators at the beginning of a line

pal/src/host/linux-sgx/pal_devices.c line 927 at r7 (raw file):

                                     MAX_MEM_REGIONS * sizeof(struct mem_region);

    /* typical IOCTL case: deep-copy the IOCTL argument's input data outside of enclave, execute the

What is the other, non-typical case?

mkow

Reviewed 1 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 73 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

-- commits line 8 at r3:

ioctl arguments are passed as-is from the app to the untrusted host, without encryption

This IMO sounds misleading, encryption wouldn't change anything if the destination is the host. It may be read as "if you do a TLS tunnel with the driver then you're fine".

In particular, ioctl arguments should be encrypted or integrity-protected with a key pre-shared between Gramine and the device.

This isn't always true, depends on the specific case. Please change to "In most cases" or something like that.

Otherwise looks good.

-- commits line 10 at r7:
Wouldn't it be a better design to have this in LibOS? I.e. not backend-specific.

Code quote:

On the Linux-SGX PAL

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Let me keep it as-is for now and wait for others' opinions (if any)

I'd go with separate names, but maybe size and array_len / array_cnt instead of what you proposed to make the distinction more obvious (users may not know our source code convention about "size" meaning "byte-size").

Documentation/manifest-syntax.rst line 767 at r7 (raw file):

    sgx.ioctl_structs.[identifier] = [memory-layout-format]

    sgx.allowed_ioctls.[identifier].request_code = [NUM]

What's the point of this ID? Shouldn't this be an array?

Code quote:

sgx.allowed_ioctls.[identifier].request_code = [NUM]

Documentation/manifest-syntax.rst line 775 at r7 (raw file):

Each IOCTL entry may also contain a reference to an IOCTL struct in the struct field.

What happens if it doesn't have it?

Documentation/manifest-syntax.rst line 812 at r7 (raw file):

  default. Unit of measurement must be a constant integer. For example,
  ``size = "strlen"`` and ``unit = 2`` denote a wide-char string (where each
  character is 2B long) of a dynamically calculated length.

Suggestion:

dynamically specified length

Documentation/manifest-syntax.rst line 887 at r7 (raw file):

   themselves in SGX environments:

       - IOCTL arguments are passed as-is from the app to the untrusted host,

ditto

Code quote:

       - IOCTL arguments are passed as-is from the app to the untrusted host,
         without encryption; this may lead to leaks of enclave data.

Documentation/manifest-syntax.rst line 890 at r7 (raw file):

         without encryption; this may lead to leaks of enclave data.
       - Untrusted host can change IOCTL arguments as it wishes when passing
         them from Gramine to the device and back.

ditto

boryspoplawski

Reviewable status: all files reviewed, 73 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

-- commits line 10 at r7:

Previously, mkow (Michał Kowalczyk) wrote…

Wouldn't it be a better design to have this in LibOS? I.e. not backend-specific.

Idk, this whole feature doesn't make much sense, if the host isn't Linux...

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd go with separate names, but maybe size and array_len / array_cnt instead of what you proposed to make the distinction more obvious (users may not know our source code convention about "size" meaning "byte-size").

I'll copy my comment from code part, so we don't split this discussion:

"So, to continue the discussion from docs, this is a bit confusing. Maybe we should have size mean the size of the pointer and a new length parameter, which would describe length of the array?"

Documentation/manifest-syntax.rst line 767 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the point of this ID? Shouldn't this be an array?

You need to somehow tie struct to ioctl request code. So either this or and array of pairs (which I guess means array of 2-size lists in TOML). IMO this is more readable.

Documentation/manifest-syntax.rst line 775 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Each IOCTL entry may also contain a reference to an IOCTL struct in the struct field.

What happens if it doesn't have it?

It's passed as is (i.e. just the value is forwarded to host, whatever that value is). I swear it was described somewhere....

mkow

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

-- commits line 10 at r7:

Previously, boryspoplawski (Borys Popławski) wrote…

Idk, this whole feature doesn't make much sense, if the host isn't Linux...

Hmmm, good point. But it's also not really SGX-specific, rather TEE-specific. Maybe the code should go to Linux-common then?
But I won't block here, I missed that it's indeed host-Linux-specific.

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I'll copy my comment from code part, so we don't split this discussion:

"So, to continue the discussion from docs, this is a bit confusing. Maybe we should have size mean the size of the pointer and a new length parameter, which would describe length of the array?"

That also sounds good to me.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):
Hmm, both versions sound equally understandable to me. I agree that using name "object" is a bit confusing when used for a padding field. Maybe just use "chunk" or something like this instead of "region"?

There is no benefit in splitting into four sub-regions.

I would say otherwise, that there is no much gain in joining them into one. I think most people will use some kind of script to translate the structs into this description (or maybe even we should provide one in the future) and even doing this manually isn't much of a problem.

Hmm, this discussion is rather orthogonal to the naming issue? It's just about wondering how users will describe their structs with the currently available syntax?

Documentation/manifest-syntax.rst line 767 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

You need to somehow tie struct to ioctl request code. So either this or and array of pairs (which I guess means array of 2-size lists in TOML). IMO this is more readable.

But here you need to invent some unique name for each entry, which e.g. for old trusted files/mounts syntax was quite annoying (e.g. it was very easy to make copy-paste errors).
Why not an array of pairs? Using [[ ]] TOML syntax it should be quite readable? The example from the docs would change this way:

sgx.allowed_ioctls.io1.request_code = 0x12345678
sgx.allowed_ioctls.io1.struct = "ioctl_read"

sgx.allowed_ioctls.io2.request_code = 0x87654321
sgx.allowed_ioctls.io2.struct = "ioctl_read"

->

[[sgx.allowed_ioctls]]
request_code = 0x12345678
struct = "ioctl_read"

[[sgx.allowed_ioctls]]
request_code = 0x87654321
struct = "ioctl_read"

Documentation/manifest-syntax.rst line 775 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It's passed as is (i.e. just the value is forwarded to host, whatever that value is). I swear it was described somewhere....

Yeah, it is, but this place alone sounds weird. I'd add something like "in case the third (or whichever it was) ioctl argument is intended to be translated by Gramine" to the end.

boryspoplawski

Reviewable status: all files reviewed, 85 unresolved discussions, not enough approvals from maintainers (1 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)

pal/src/host/linux-sgx/pal_devices.c line 385 at r7 (raw file):

/* finds a sub region with name `sub_region_name` among `sub_regions` and reads the value in it */
static int get_sub_region_value(struct sub_region* sub_regions, size_t sub_regions_cnt,

This function actually gets only value of regions which is a size of another region (or more precisely an integer). The name does not imply this at all.

pal/src/host/linux-sgx/pal_devices.c line 396 at r7 (raw file):

                          sub_regions[idx].name_hash, sub_region_name_hash)) {
            /* found corresponding sub region, read its value */
            if (!sub_regions[idx].encl_addr || sub_regions[idx].encl_addr == (void*)-1) {

How can this even this addr be -1? Why don't you check -2 too?

Code quote:

-1

pal/src/host/linux-sgx/pal_devices.c line 413 at r7 (raw file):

                          "legitimate size: 1, 2, 4 or 8 bytes)", sub_regions[idx].name);
                return -PAL_ERROR_INVAL;
            }

Why not

Suggestion:

            if (sub_regions[idx].size > 8) {
                log_error("blablabla", sub_regions[idx].name);
                return -PAL_ERROR_INVAL;
            }
            *out_value = 0;
            memcpy(out_value, sub_regions[idx].encl_addr, sub_region[idx].size);

pal/src/host/linux-sgx/pal_devices.c line 424 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This function is quite big. Could it be split into multiple? I've not reviewed it yet, so maybe it makes no sense to do so.

Ok, I give up, it's impossible to reason about variables, their lifetime and what is what, it's too long. Please refactor/split it.
I did more general review of it, without the details for now

pal/src/host/linux-sgx/pal_devices.c line 435 at r7 (raw file):

    assert(pal_get_enclave_tcb()->ioctl_scratch_space);
    struct mem_region* mem_regions = (struct mem_region*)pal_get_enclave_tcb()->ioctl_scratch_space;

Why this function gets sub_regions as an argument (which is this scratch space + some offset) and mem_regions directly from tcb? Please unify (preferably both as arguments).

pal/src/host/linux-sgx/pal_devices.c line 452 at r7 (raw file):

            cur_encl_addr = cur_mem_region->encl_addr;

        size_t cur_mem_region_first_sub_region = sub_regions_cnt;

This is not a region, but an index? Even to not yet populated region, I believe

Code quote:

cur_mem_region_first_sub_region

pal/src/host/linux-sgx/pal_devices.c line 453 at r7 (raw file):

        size_t cur_mem_region_first_sub_region = sub_regions_cnt;

Please add assert(toml_array_nelem(cur_mem_region->toml_array) >= 0);
Actually is it even checked anywhere?

pal/src/host/linux-sgx/pal_devices.c line 477 at r7 (raw file):

            cur_sub_region->encl_addr = cur_encl_addr;
            if (!cur_encl_addr || cur_encl_addr == (void*)-1) {

How can this addr be -1?

pal/src/host/linux-sgx/pal_devices.c line 480 at r7 (raw file):

                /* enclave address is invalid, user provided bad struct */
                ret = -PAL_ERROR_DENIED;
                goto out;

Isn't name and name_size undefined here? It will be freeed after this goto out.
This applies to many other gotos in this function. Please just split it into multiple smaller ones, will be much easier to spot such problems.

pal/src/host/linux-sgx/pal_devices.c line 507 at r7 (raw file):

                    if (!strcmp(sub_region_name, "this")) {
                        /* special case of `{ptr="this"}` -- ptr to object of the IOCTL root type */
                        sub_region_ptr_arr = root_toml_array;

Hmmm, so this doesn't mean this struct/memory region, but the first one defined? What if:

struct A {
    struct B* ptr;
};

struct B {
   ...
    struct B* this;
};

pal/src/host/linux-sgx/pal_devices.c line 510 at r7 (raw file):

                    } else {
                        size_t idx;
                        ret = get_sub_region_idx(sub_regions, sub_regions_cnt, sub_region_name,

Wait, isn't sub_region_name actually a name of another memory region?

pal/src/host/linux-sgx/pal_devices.c line 511 at r7 (raw file):

                        size_t idx;
                        ret = get_sub_region_idx(sub_regions, sub_regions_cnt, sub_region_name,
                                hash(sub_region_name), &idx);

misaligned

pal/src/host/linux-sgx/pal_devices.c line 514 at r7 (raw file):

                        if (ret < 0) {
                            log_error("Invalid deep-copy syntax (cannot find sub region '%s')",
                                    sub_region_name);

misaligned

boryspoplawski

Reviewable status: all files reviewed, 85 unresolved discussions, not enough approvals from maintainers (1 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 and @mkow)

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, both versions sound equally understandable to me. I agree that using name "object" is a bit confusing when used for a padding field. Maybe just use "chunk" or something like this instead of "region"?

There is no benefit in splitting into four sub-regions.

I would say otherwise, that there is no much gain in joining them into one. I think most people will use some kind of script to translate the structs into this description (or maybe even we should provide one in the future) and even doing this manually isn't much of a problem.

Hmm, this discussion is rather orthogonal to the naming issue? It's just about wondering how users will describe their structs with the currently available syntax?

But depending on how you describe the struct, the naming might change (e.g. object would make 0 sense if it was supposed to describe two consecutive fields of a struct). So not really orthogonal.

dimakuv

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

-- commits line 8 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

ioctl arguments are passed as-is from the app to the untrusted host, without encryption

This IMO sounds misleading, encryption wouldn't change anything if the destination is the host. It may be read as "if you do a TLS tunnel with the driver then you're fine".

In particular, ioctl arguments should be encrypted or integrity-protected with a key pre-shared between Gramine and the device.

This isn't always true, depends on the specific case. Please change to "In most cases" or something like that.

Otherwise looks good.

@mkow What about this improved text.

  The newly added `ioctl()` syscall emulation on device-backed file
  descriptors is pass-through. It is insecure by itself since the
  emulation only passes the arguments to and from the untrusted memory:
    - ioctl arguments are passed as-is from the app to the untrusted host,
      which may lead to leaks of secret data;
    - untrusted host can change ioctl arguments as it wishes when passing
      them from Gramine to the device and back.

  It is the responsibility of the app developer to correctly use ioctls,
  with security implications in mind. In most cases, ioctl arguments should
  be encrypted or integrity-protected with a key pre-shared between
  Gramine and the device.

-- commits line 10 at r7:

Previously, mkow (Michał Kowalczyk) wrote…

Hmmm, good point. But it's also not really SGX-specific, rather TEE-specific. Maybe the code should go to Linux-common then?
But I won't block here, I missed that it's indeed host-Linux-specific.

I suggest I continue with the PR as-is for now, and either before merge or after merge (in a follow-up PR) I can move to e.g. Linux-common.

Documentation/manifest-syntax.rst line 798 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

That also sounds good to me.

Done. Now we have size for normal objects (non-pointers) and array_len for arrays (pointers).

I see no reason to have an additional size keyword for pointers/arrays, because they are currently always 8 bytes in size. So I simply error out when I detect this keyword on pointers.

Documentation/manifest-syntax.rst line 826 at r3 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

But depending on how you describe the struct, the naming might change (e.g. object would make 0 sense if it was supposed to describe two consecutive fields of a struct). So not really orthogonal.

I don't like the word "chunk". Chunks are fixed-sized in my mind, and that's not true in our case.

I'm still leaning towards my current "region" + "sub-region" terminology. By the way, if we would go with "objects", would we have "objects" and "sub-objects"? Or would we have "objects" and "object fields"?

Documentation/manifest-syntax.rst line 767 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But here you need to invent some unique name for each entry, which e.g. for old trusted files/mounts syntax was quite annoying (e.g. it was very easy to make copy-paste errors).
Why not an array of pairs? Using [[ ]] TOML syntax it should be quite readable? The example from the docs would change this way:
sgx.allowed_ioctls.io1.request_code = 0x12345678
sgx.allowed_ioctls.io1.struct = "ioctl_read"

sgx.allowed_ioctls.io2.request_code = 0x87654321
sgx.allowed_ioctls.io2.struct = "ioctl_read"
->
[[sgx.allowed_ioctls]]
request_code = 0x12345678
struct = "ioctl_read"

[[sgx.allowed_ioctls]]
request_code = 0x87654321
struct = "ioctl_read"

Done.

Documentation/manifest-syntax.rst line 775 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Yeah, it is, but this place alone sounds weird. I'd add something like "in case the third (or whichever it was) ioctl argument is intended to be translated by Gramine" to the end.

Done.

Documentation/manifest-syntax.rst line 812 at r7 (raw file):

  default. Unit of measurement must be a constant integer. For example,
  ``size = "strlen"`` and ``unit = 2`` denote a wide-char string (where each
  character is 2B long) of a dynamically calculated length.

Done.

Documentation/manifest-syntax.rst line 887 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

Documentation/manifest-syntax.rst line 890 at r7 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto

Done.

libos/test/regression/device_ioctl.c line 1 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Actually, shouldn't we use off_t for user facing API (ioctls)? TBH I not sure what are the rules here, which type should be used where...

From what I gather, loff_t is fine to use in userspace apps.

libos/test/regression/device_ioctl.manifest.template line 13 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It's set in mnifest.template in LibOS regression tests, because we have one template for multiple tests and some of them need this option. Here it's not needed.

Tried to remove it, but I can't -- GCC (at least current GCC on my Ubuntu 20.04) creates a non-PIE executable by default. I can change this in meson.build of course, but do we want to?

libos/test/regression/device_ioctl.manifest.template line 23 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We still support this syntax. I see no downside in supporting this special case.

Done (removed)

libos/test/regression/device_ioctl.manifest.template line 27 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Yes, I don't see why anybody would want to use it (create some dummy structs), if they can simply write "" or even omit this field.

Done.

libos/test/regression/device_ioctl.manifest.template line 66 at r6 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

If it's for testing, then maybe do size = 1, units = 2, because units = 1 is the default

Done.

libos/test/regression/meson.build line 19 at r6 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This path is included by default

It's not included for Musl builds. In fact, that's how I found out I need this include explicitly (otherwise Musl build failed).

pal/include/pal/pal.h line 874 at r5 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

comma still there

Done. Though I was taught to always put comma after e.g. and i.e..

pal/src/host/linux/pal_devices.c line 224 at r5 (raw file):

No, I was just confused why it returns device-specific return values via out_ret but error codes, which can also be device-specific, directly.

Ah, I see your point. You mean that error codes can be also considered as "IOCTL return values", and thus shouldn't have special treatment.

I think this breaks Gramine conventions? Gramine always expects Pal functions to return PAL error codes (and everything less than 0 is considered an error code), so I don't know how to make it better than currently.

pal/src/host/linux-sgx/pal_devices.c line 225 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This read weirdly for me (deep-copy syntax?), maybe:

Done.

pal/src/host/linux-sgx/pal_devices.c line 228 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would add a note here, that "high level"/"syntax" description is in "manifest syntax" docs.

Done.

pal/src/host/linux-sgx/pal_devices.c line 236 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Just so you are able to refer it below

Done.

pal/src/host/linux-sgx/pal_devices.c line 249 at r7 (raw file):

 * occurences of chars "x" and "y" in the Pascal string.
 *
 * The corresponding deep-copy syntax in TOML looks like this:

Done.

pal/src/host/linux-sgx/pal_devices.c line 254 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This looks wrong to me. Am I missing something?

Done, removed. I just wanted to highlight as an example that this is an "array of length 1".

pal/src/host/linux-sgx/pal_devices.c line 255 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

These make it only more confusing. This is not docs trying to describe all possible options, so I would just remove them

Done.

pal/src/host/linux-sgx/pal_devices.c line 256 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wait, why is this size = 2?

Done. Yes, it was a mistake.

pal/src/host/linux-sgx/pal_devices.c line 257 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

pal/src/host/linux-sgx/pal_devices.c line 260 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

If you rename above then you could refer to it also here.
Note that in your example DEVICE_MAGIC is just some constant, which is used as argument to ioctl-number creating macros (like _IOW), so this is a misleading.

Done.

pal/src/host/linux-sgx/pal_devices.c line 268 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

It can also be a name of another struct/memory region, right?

Done.

pal/src/host/linux-sgx/pal_devices.c line 269 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Actually, why is size hardcoded? 8B is a good default, but why it cannot be overwritten explicitly?

What is the point of overwriting the pointer size? Why should we allow this?

pal/src/host/linux-sgx/pal_devices.c line 270 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

So, to continue the discussion from docs, this is a bit confusing. Maybe we should have size mean the size of the pointer and a new length parameter, which would describe length of the array?

Done. But please note that I do not allow size together with ptr -- I hard-code size to 8.

pal/src/host/linux-sgx/pal_devices.c line 275 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this limitation? (<= 8 I would understand)
Don't you memcpy the size anyway?

Done.

pal/src/host/linux-sgx/pal_devices.c line 279 at r7 (raw file):

 *     sub-regions but may be omitted for all other kinds of sub-regions.
 *  7. Sub-regions may have one of the four directions: "out" to copy contents of the sub-region
 *     outside the enclave to untrusted memory, "in" to copy from untrusted memory to inside the

Done.

pal/src/host/linux-sgx/pal_devices.c line 279 at r7 (raw file):

 *     sub-regions but may be omitted for all other kinds of sub-regions.
 *  7. Sub-regions may have one of the four directions: "out" to copy contents of the sub-region
 *     outside the enclave to untrusted memory, "in" to copy from untrusted memory to inside the

Done.

pal/src/host/linux-sgx/pal_devices.c line 282 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Actually the pointer region can be an input argument, so it wouldn't be "copied-out"

Sorry, I don't get your comment. When I talk about pointer sub-regions, I mean literally the 8-byte memory cell where the address is stored (the pointer value).

pal/src/host/linux-sgx/pal_devices.c line 299 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

?

Done, removed zeros. I wanted to highlight that these values are not copied out from the enclave, and thus have default values (zeros). But I guess this is only confusing.

pal/src/host/linux-sgx/pal_devices.c line 322 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please split into separate lines.

Done.

pal/src/host/linux-sgx/pal_devices.c line 322 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This _ENCLAVE suffix makes little sense to me (I assume it means "copy out of the enclave", but not sure).
IMO better alternatives:

DIRECTION_NONE (my preference out of these),

COPY_NONE,

MEM_COPY_NONE.

Done.

dimakuv

Reviewable status: 16 of 22 files reviewed, 85 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

pal/src/host/linux-sgx/pal_devices.c line 252 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add more spaces (especially around =).

Done.

pal/src/host/linux-sgx/pal_devices.c line 332 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

misaligned comment

Done. That's on purpose because other comments then will exceed 100-char-limit. I think it is ok in this case.

pal/src/host/linux-sgx/pal_devices.c line 335 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why signed?

Done.

pal/src/host/linux-sgx/pal_devices.c line 336 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done. Reworked this into a struct dynamic_value.

pal/src/host/linux-sgx/pal_devices.c line 339 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

pal/src/host/linux-sgx/pal_devices.c line 343 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I'm surprised. Shouldn't this be struct mem_region*?
I've not yet read rest of the code, but from the description I would expect it

Well, not, because we create struct mem_region* on the fly.

pal/src/host/linux-sgx/pal_devices.c line 346 at r7 (raw file):

};

static inline uint64_t hash(char* str) {

Done. I removed the string hashes for now, to simplify the PR. This was giving me some substantial perf benefits, so I may re-introduce it in follow-up PRs. I left some FIXME comments to remember this.

pal/src/host/linux-sgx/pal_devices.c line 351 at r7 (raw file):

    char c;
    while ((c = *str++))
        hash = ((hash << 5) + hash) + c;

Done. Not relevant.

pal/src/host/linux-sgx/pal_devices.c line 358 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

We literally check this in the line above...

Done. Not relevant.

pal/src/host/linux-sgx/pal_devices.c line 367 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I don't understand this. Maybe it gets cleaner when I read the usage.

Should I expand the comment? This is basically for the cases of arrays: then each new sub region has the same name (the array item name), but when we call this function, we need to get the last item of the array. Well, I don't know how to explain it exactly, it's like what IOCTL structs do.

pal/src/host/linux-sgx/pal_devices.c line 374 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Description above, and more importantly the name of this function do not imply this requirement...

Done. You're right, I moved this check out of this func, it doesn't belong here but to the caller.

pal/src/host/linux-sgx/pal_devices.c line 385 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This function actually gets only value of regions which is a size of another region (or more precisely an integer). The name does not imply this at all.

Done. This function was removed, and its usages were inlined.

pal/src/host/linux-sgx/pal_devices.c line 396 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

How can this even this addr be -1? Why don't you check -2 too?

Done. Removed it. It was for some LTP tests (iirc), because they tend to supply -1 as a wrong pointer, just for testing.

pal/src/host/linux-sgx/pal_devices.c line 413 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why not

This will incorrect copy the bytes (imagine copying 1-byte value from sub_regions[idx].encl_addr into 8-byte out_value). I'm not sure what should be the correct way of copying, so I have this stupid code.

pal/src/host/linux-sgx/pal_devices.c line 422 at r7 (raw file):

}

/* caller sets `sub_regions_cnt_ptr` to maximum number of sub_regions; this variable is updated to

Done.

pal/src/host/linux-sgx/pal_devices.c line 424 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Ok, I give up, it's impossible to reason about variables, their lifetime and what is what, it's too long. Please refactor/split it.
I did more general review of it, without the details for now

Done. The func collect_sub_regions is still a bit long, but I think most of its code is trivial.

pal/src/host/linux-sgx/pal_devices.c line 429 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I think this code works correctly without this assumption, I would remove it.

Done

pal/src/host/linux-sgx/pal_devices.c line 435 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this function gets sub_regions as an argument (which is this scratch space + some offset) and mem_regions directly from tcb? Please unify (preferably both as arguments).

Done.

pal/src/host/linux-sgx/pal_devices.c line 452 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is not a region, but an index? Even to not yet populated region, I believe

Done. Yes, your understanding is correct. This is basically to do a second run through the sub-regions of the current memory region, to resolve the unresolved "array len". (In C terms, to run through the fields of the struct again.)

pal/src/host/linux-sgx/pal_devices.c line 453 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add assert(toml_array_nelem(cur_mem_region->toml_array) >= 0);
Actually is it even checked anywhere?

What's the point of this assert? toml_array_nelem() always returns non-negative value if cur_mem_region->toml_array object exists. And it exists:

On this func entry, the first mem region's toml_array is equal to root_toml_array
On other iterations, the mem region's toml_array is set to sub_regions[i].mem_ptr, which is guaranteed to be set

pal/src/host/linux-sgx/pal_devices.c line 477 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

How can this addr be -1?

Done. Removed, this one for some LTP tests which used -1 for "bad pointer values"

pal/src/host/linux-sgx/pal_devices.c line 480 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Isn't name and name_size undefined here? It will be freeed after this goto out.
This applies to many other gotos in this function. Please just split it into multiple smaller ones, will be much easier to spot such problems.

Done. I now explicitly memset(0), so name = NULL.

Also split into small functions.

pal/src/host/linux-sgx/pal_devices.c line 507 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Hmmm, so this doesn't mean this struct/memory region, but the first one defined? What if:
struct A {
    struct B* ptr;
};

struct B {
   ...
    struct B* this;
};

Yep, we don't support this. "this" always forces to point to the root IOCTL struct. I spell it out explicitly in the documentation (in manifest-syntax.rst).

pal/src/host/linux-sgx/pal_devices.c line 510 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wait, isn't sub_region_name actually a name of another memory region?

Well, not exactly. Memory regions do not have names. But their sub-regions do. So here we want to find a ptr sub region with sub_region_name.

In other words, we can do this kind of stuff:

sgx.ioctl_structs.bla = [
    { name = "first-pointer-to-mem-region", ptr = [ {size = 256, direction = "out"} ] },
    { ptr = "first-pointer-to-mem-region" },
]

Here we'll have two pointers in this bla ioctl struct. Both will point to the same object (a 256-byte buffer) in memory. Same as in C:

struct bla bla1 = {.first_ptr = &buf, .second_ptr = &buf }

If you think that sub_region_name is misleading word, I can replace with... hm... ptr_sub_region_name?

pal/src/host/linux-sgx/pal_devices.c line 511 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

misaligned

Done. Not relevant.

pal/src/host/linux-sgx/pal_devices.c line 514 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

misaligned

Done. Not relevant.

pal/src/host/linux-sgx/pal_devices.c line 737 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why is it possible to have encl_addr == NULL and size > 0?

This is not possible. But the other case is possible: encl_addr = 0x1234 and size == 0. This can happen if the size is dynamically calculated to zero.

pal/src/host/linux-sgx/pal_devices.c line 747 at r7 (raw file):

        if (sub_regions[i].direction == COPY_OUT_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE) {

Done.

pal/src/host/linux-sgx/pal_devices.c line 748 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please use sgx_copy_from_enclave

Done. We don't have such a function, but I added checks for in-enclave-memory-region and outside-memory-region.

pal/src/host/linux-sgx/pal_devices.c line 758 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

pal/src/host/linux-sgx/pal_devices.c line 766 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please use sgx_copy_from_enclave

Done. We already verified them above. Added a comment.

pal/src/host/linux-sgx/pal_devices.c line 776 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

pal/src/host/linux-sgx/pal_devices.c line 780 at r7 (raw file):

        if (sub_regions[i].direction == COPY_IN_ENCLAVE ||
                sub_regions[i].direction == COPY_INOUT_ENCLAVE)

Done.

pal/src/host/linux-sgx/pal_devices.c line 780 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would also prefer { } since this is a multiline condition, but just personal preference (not blocking)

Done

pal/src/host/linux-sgx/pal_devices.c line 781 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

sgx_copy_to_enclave

Done.

pal/src/host/linux-sgx/pal_devices.c line 802 at r7 (raw file):

        return -PAL_ERROR_NOTIMPLEMENTED;

    for (ssize_t idx = 0; idx < toml_allowed_ioctls_cnt; idx++) {

Done.

pal/src/host/linux-sgx/pal_devices.c line 807 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Is this even possible?

Done. Yes, there could be non-table items in the TOML array (non-homogeneous TOML array).

I currently skip such unrecognized array items + write a log message. Should I just error out?

pal/src/host/linux-sgx/pal_devices.c line 816 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

From where comes the assumption that 0 is invalid ioctl request code?

Done. Yes, this doesn't make sense. But the request code should be non-negative.

pal/src/host/linux-sgx/pal_devices.c line 820 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why not toml_int_in?

Done.

pal/src/host/linux-sgx/pal_devices.c line 822 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I think moving the whole inside of this if to a separate function would make it more readable.

Done.

pal/src/host/linux-sgx/pal_devices.c line 873 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why do we even need this?
It's around 1MB of PAL internal memory per thread...
Also this code is O(#sub_regions^2) (or more, didn't review all of it), so I wouldn't expect allocations to worsen performance that much...

Done. This was premature optimization. I guess I won't even re-introduce it in any follow-up PRs, since it gave quiet small benefits.

pal/src/host/linux-sgx/pal_devices.c line 874 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This + is weird.

Done, not relevant anymore.

pal/src/host/linux-sgx/pal_devices.c line 886 at r7 (raw file):

    size_t total_size = MAX_MEM_REGIONS * sizeof(struct mem_region) +
                            MAX_SUB_REGIONS * sizeof(struct sub_region);

Done, not relevant anymore.

pal/src/host/linux-sgx/pal_devices.c line 925 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is wrong. You actually add MAX_MEM_REGIONS * sizeof(struct mem_region) * sizeof(struct sub_region).
Also we put dangling operators at the beginning of a line

Done, not relevant anymore.

pal/src/host/linux-sgx/pal_devices.c line 927 at r7 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What is the other, non-typical case?

Done.

mkow

Reviewable status: 23 of 24 files reviewed, 28 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

.ci/lib/stage-build-sgx-vm.jenkinsfile line 12 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Previously, I just ran the tests using gramine-direct helloworld/gramine-sgx helloworld and relied on the fact that they return 0 on success. That's why I had this helper variable GRAMINE.

Now I'm simply re-using the gramine-test pytest functionality to run these tests. I had to do this for two reasons:

It is a cleaner approach to run tests then to just run raw binaries.

I introduced the new test called device_ioctl_fail where the correct result is the failure of the test, and that the test prints a specific failure message. This would be hard to verify using simple bash and running a raw binary. So it dictated the use of gramine-test pytest.

Now how does gramine-test pytest know whether it should run gramine-direct or gramine-sgx? Well, we propagate the env variable SGX into the VM and ultimately into this bash script:

gramine/.ci/lib/stage-test-vm.jenkinsfile

Line 9 in c7fe3f0

./run.sh PWD_FOR_VM=$PWD_FOR_VM SGX=$SGX IS_VM=$IS_VM PATH=/sbin:$PATH \

Also, gramine-test tool itself knows about this SGX variable and chooses the correct flows when SGX=1:

gramine/python/gramine-test

Line 19 in c7fe3f0

@click.option('--sgx/--no-sgx', default=(os.environ.get('SGX') == '1'),

Ah, didn't know gramine-test also respects this env variable.

libos/test/regression/device_ioctl.c line 29 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't device passthrough make all FD operations also pass through?

Well, that's currently not implemented and that's not the purpose of this PR. But I agree, it would be better to have such a passthrough, though it would require some non-trivial changes to the LibOS FS subsystem.

Do you want me to create a GitHub issue about this?

If you think it's indeed better then please create an issue + document the limitation in the documentation in this PR.

libos/test/regression/test_libos.py line 1048 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I felt that this way would be more readable, but sure.

I'm used to \( and I had to stop and think for a moment when I saw [(].

pal/src/host/linux-sgx/pal_devices.c line 228 at r20 (raw file):

 * Code below describes the TOML syntax of the manifest entries used for deep-copying complex nested
 * objects out and inside the SGX enclave. This syntax is currently used for IOCTL emulation. This
 * syntax is generic enough to describe any memory layout for deep copy of IOCTL structs.

You can't describe any possible data layout with the current format. Just the most common ones.

Code quote:

is generic enough to describe any memory layout for deep copy of IOCTL structs.

mkow

Reviewed 1 of 1 files at r15.
Reviewable status: all files reviewed, 49 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

a discussion (no related file):
@kailun-qin, could you also take a look at this PR? The parsing code is so complex and monotonous that I don't trust my review.

libos/test/regression/test_libos.py line 1050 at r20 (raw file):

            self.assertRegex(stdout, r'ioctl[(]devfd, GRAMINE_TEST_DEV_IOCTL_REWIND[)].*'
                                     r'Function not implemented')

Could you add some negative tests for parsing of these structs? So that ASan gets more coverage - e.g. wrong types of elements, empty arrays/tables, negative or super big numbers, etc.).
The parsing logic is really complex and the only tests we have are super simple.

pal/src/host/linux-sgx/pal_devices.c line 269 at r7 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I still feel like it's shooting ourselves in the foot. Currently I error out with a loud error message when I see { ptr = ..., size = ...}.

If we allow { ptr = ..., size = 5}, then it will sometimes work (when addr value fits into 5 bytes), and sometimes not, so good luck debugging such bloopers...

I'm rather neutral on this, but we'll need to change it when moving to a more generic place.

pal/src/host/linux-sgx/pal_devices.c line 236 at r20 (raw file):

 *   struct pascal_str { uint8_t len; char str[]; };
 *   struct c_str { char str[]; };
 *   struct root { struct pascal_str* s1; struct c_str* s2; uint64_t s2_len; int8_t x; int8_t y; };

It's not really a C-string if you represent it as a pointer and length? ;)

Code quote:

struct c_str* s2; uint64_t s2_len

pal/src/host/linux-sgx/pal_devices.c line 244 at r20 (raw file):

 * contains two pointers to other objects (pascal-style string and a C-style string) and embeds two
 * integers `x` and `y`. The two strings reside in separate memory regions in enclave memory. Note
 * that the max possible length of the C-style string is stored in the `s2_len` field of the root

Why is it called "s2 len" then, if it's not the length of the string, but the buffer allocation size?

Code quote:

the max possible length of the C-style string is stored in the `s2_len` field

pal/src/host/linux-sgx/pal_devices.c line 271 at r20 (raw file):

 *   ]
 *
 * One can observe the following rules in this TOML syntax:

Suggestion:

The current TOML syntax has the following rules/limitations:

pal/src/host/linux-sgx/pal_devices.c line 278 at r20 (raw file):

 *     `ptr` is a TOML-array representation of that other memory region or a TOML string with the
 *     name of another memory region. The `ptr` sub-region always has size of 8B (assuming x86-64)
 *     and doesn't have an in/out direction.  The `array_len` field specifies the number of adjacent

I think we decided to not do double-space-after-period at some point? I'm neutral on this, but would like to be consistent.

Code quote:

pal/src/host/linux-sgx/pal_devices.c line 279 at r20 (raw file):

 *     name of another memory region. The `ptr` sub-region always has size of 8B (assuming x86-64)
 *     and doesn't have an in/out direction.  The `array_len` field specifies the number of adjacent
 *     memory regions that this pointer points to (i.e. the length of an array).

Suggestion:

i.e. the length of the array

pal/src/host/linux-sgx/pal_devices.c line 287 at r20 (raw file):

 *     sub-region, either in the same memory region (e.g. as in flexible array members in C) or in
 *     the "outer" memory region (e.g. the size specifier is located in the root memory region and
 *     the buffer is located in the nested memory region).

This is just a limitation of the current parser, right? Normally if the array is referenced via a pointer then it should be fine, but a bit harder to parse (I guess) if the size is located after the pointer?
If that's the case, would be good to include this in the comment.

pal/src/host/linux-sgx/pal_devices.c line 335 at r20 (raw file):

or a special "pointer" sub-region

Stale comment? There's no such value in this enum.

pal/src/host/linux-sgx/pal_devices.c line 344 at r20 (raw file):

struct mem_region {
    toml_array_t* toml_mem_region; /* describes contigious sub_regions in this mem_region */

Suggestion:

contiguous

pal/src/host/linux-sgx/pal_devices.c line 344 at r20 (raw file):

struct mem_region {
    toml_array_t* toml_mem_region; /* describes contigious sub_regions in this mem_region */

Suggestion:

sub-regions in this mem-region

pal/src/host/linux-sgx/pal_devices.c line 440 at r20 (raw file):

    int64_t align;
    int ret = toml_int_in(toml_sub_region, "align", /*defaultval=*/0, &align);
    if (ret < 0 || align < 0)

Suggestion:

align <= 0

pal/src/host/linux-sgx/pal_devices.c line 462 at r20 (raw file):

}

static int get_toml_mem_region(toml_table_t* toml_sub_region, toml_array_t** out_toml_mem_region) {

The current name was quite confusing for me, I had to read its code to get what it actually does.

Suggestion:

get_toml_nested_mem_region

pal/src/host/linux-sgx/pal_devices.c line 479 at r20 (raw file):

    }

    /* since we're in this function, we are parsing sgx.ioctl_structs list, so we know it exists */

Wouldn't it be cleaner (and faster) to pass the corresponding TOML object as an argument to this function? (instead of searching for it again)

Code quote:

/* since we're in this function, we are parsing sgx.ioctl_structs list, so we know it exists */

pal/src/host/linux-sgx/pal_devices.c line 517 at r20 (raw file):

    }

    /* size must be specified as string (another sub-region's name) */

Suggestion:

/* size must have been specified as string (another sub-region's name) */

pal/src/host/linux-sgx/pal_devices.c line 531 at r20 (raw file):

    void* addr_of_size_field  = all_sub_regions[found_idx].enclave_addr;
    size_t size_of_size_field = all_sub_regions[found_idx].size;
    return copy_value(addr_of_size_field, size_of_size_field, out_size);

I don't like this implicit pointer conversion (copy_value() expects uint64_t*, but gets size_t* here). I'd rather read to uint64_t and then convert the value to size_t + a static assert for sizes.

Code quote:

return copy_value(addr_of_size_field, size_of_size_field, out_size);

pal/src/host/linux-sgx/pal_devices.c line 588 at r20 (raw file):

    size_t mem_regions_cnt = 0;

    mem_regions[0].toml_mem_region = root_toml_mem_region;

BOF if max_mem_regions == 0 (I know that in the current code it can't happen, but it's still a bad and dangerous practice)

pal/src/host/linux-sgx/pal_devices.c line 595 at r20 (raw file):

    /* collecting memory regions and their sub-regions must use top-to-bottom breadth-first search
     * to dynamically calculate sizes of sub-regions even if they are specified via another
     * sub-region's "name" */

This sentence would be more useful if it said why you have to use BFS. Now it's only "I have to use BFS despite [...]".

pal/src/host/linux-sgx/pal_devices.c line 770 at r20 (raw file):

                void* mem_region_addr = *((void**)sub_regions[i].enclave_addr);
                if (!mem_region_addr)

Is this a bug? The value of this condition can't change between iterations...

Code quote:

                void* mem_region_addr = *((void**)sub_regions[i].enclave_addr);
                if (!mem_region_addr)

pal/src/host/linux-sgx/pal_devices.c line 789 at r20 (raw file):

        free(sub_regions[i].name);
        if (!sub_regions[i].array_len.is_determined)
            free(sub_regions[i].array_len.sub_region_name);

sub_regions will be used afterwards by the caller, so please zero all freed pointers (here and above).

pal/src/host/linux-sgx/pal_devices.c line 794 at r20 (raw file):

}

static int copy_sub_regions_to_untrusted(struct sub_region* sub_regions, size_t sub_regions_cnt,

Everything here and below needs fixing against ÆPIC Leak, we can't just memset/memcpy memory in and out, unfortunately.
Note to myself: continue reviewing from this point once this is fixed.

dimakuv

Reviewable status: 14 of 26 files reviewed, 47 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @mkow)

-- commits line 27 at r16:

Previously, mkow (Michał Kowalczyk) wrote…

Sounds weird to me, is this a real construct in English? I'd rather say "is located" or "can be found".

Will do at final rebase.

-- commits line 27 at r16:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

TODO: Add a note on a second test (device_ioctl_fail) that tests the failure if no allowed IOCTLs.

TODO: And on the third test.

Documentation/manifest-syntax.rst line 824 at r16 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

(this is only for ioctl passthrough, not the whole Gramine)

Done.

Documentation/manifest-syntax.rst line 839 at r16 (raw file):

- ``name`` is an optional name for this sub-region; mainly used to find
  length-specifying fields and nested memory regions.
- ``align`` is an optional alignment of the memory region; may be specified only

What do you mean? The key is called align. Do you suggest to rename the key?

Documentation/manifest-syntax.rst line 841 at r16 (raw file):

- ``align`` is an optional alignment of the memory region; may be specified only
  in the first sub-region of a memory region (all other sub-regions are
  contigious with the first sub-region, so specifying their alignment doesn't

Done.

Documentation/manifest-syntax.rst line 849 at r16 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But at the beginning you say that this field is mandatory?

Done. Removed the word mandatory.

Documentation/manifest-syntax.rst line 854 at r16 (raw file):

  ``size = "strlen"`` and ``unit = 2`` denote a wide-char string (where each
  character is 2B long) of a dynamically specified length.
- ``adjust`` is an optional integer adjustment for ``size`` (always specified in

What do you mean? The key is called adjust. Do you suggest to rename the key?

Documentation/manifest-syntax.rst line 861 at r16 (raw file):

  cannot be specified with non-``ptr`` regions.
- ``direction = "none" | "out" | "in" | "inout"`` is an optional direction of
  copy for this sub-region. For example, ``direction = "out"`` denotes a

Done.

Documentation/manifest-syntax.rst line 871 at r16 (raw file):

  specifies a pointer to another, nested memory region. This field is required
  when describing complex IOCTL structs. Such pointer memory region always has
  the implicit size of 8B, and the pointer value is always rewired to the memory

Done.

Documentation/manifest-syntax.rst line 874 at r16 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto for the parenthesis

Done.

Documentation/manifest-syntax.rst line 877 at r16 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please use .. code-block:: C (but I'm not sure if this syntax is exactly correct) to highlight this snippet.

Done.

Documentation/manifest-syntax.rst line 882 at r16 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't think this is a valid C syntax?

Done.

Documentation/manifest-syntax.rst line 925 at r16 (raw file):

.. note ::
   IOCTLs for device communication are pass-through and thus insecure by

Done.

Documentation/manifest-syntax.rst line 875 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'd add that this may be recursive with NULL value being a guard, which allows describing linked lists.

Done.

libos/test/regression/device_ioctl.c line 29 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

If you think it's indeed better then please create an issue + document the limitation in the documentation in this PR.

Done: #1344

libos/test/regression/device_ioctl.manifest.template line 38 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

("pad" is a verb)

Done.

libos/test/regression/test_libos.py line 1048 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I'm used to \( and I had to stop and think for a moment when I saw [(].

Done.

libos/test/regression/test_libos.py line 1050 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Could you add some negative tests for parsing of these structs? So that ASan gets more coverage - e.g. wrong types of elements, empty arrays/tables, negative or super big numbers, etc.).
The parsing logic is really complex and the only tests we have are super simple.

Done.

pal/src/host/linux/pal_devices.c line 224 at r5 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ok, I think Borys' proposal makes sense if we're actually just passing through the whole syscall.

Done.

pal/src/host/linux-sgx/pal_devices.c line 228 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

You can't describe any possible data layout with the current format. Just the most common ones.

Done.

pal/src/host/linux-sgx/pal_devices.c line 236 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

It's not really a C-string if you represent it as a pointer and length? ;)

Done. Is that better? My point was that the user gives the buffer and its size to the IOCTL, and the IOCTL uses this info to copy the "Pascal string" object into this buffer, also updating the "size" field on return.

This is a highly contrived example...

pal/src/host/linux-sgx/pal_devices.c line 244 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is it called "s2 len" then, if it's not the length of the string, but the buffer allocation size?

Done.

pal/src/host/linux-sgx/pal_devices.c line 271 at r20 (raw file):

 *   ]
 *
 * One can observe the following rules in this TOML syntax:

Done.

pal/src/host/linux-sgx/pal_devices.c line 278 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I think we decided to not do double-space-after-period at some point? I'm neutral on this, but would like to be consistent.

Yes, this is a blooper. My Vim sometimes adds the double-space, and I didn't bother to learn how to remove this automatic thingy.

pal/src/host/linux-sgx/pal_devices.c line 279 at r20 (raw file):

 *     name of another memory region. The `ptr` sub-region always has size of 8B (assuming x86-64)
 *     and doesn't have an in/out direction.  The `array_len` field specifies the number of adjacent
 *     memory regions that this pointer points to (i.e. the length of an array).

Done.

pal/src/host/linux-sgx/pal_devices.c line 287 at r20 (raw file):
Done. Though maybe this comment is not needed, because I think there are no legitimate cases where it would be possible, see below.

a bit harder to parse (I guess) if the size is located after the pointer?

I'm not sure I understand you. The size after the pointer is not a problem -- the pointer is 8-byte-sized always, and the array this pointer points to is always a nested memory region, so it will always be "after" the size, because we do Breadth First Search (in other words, when the parser gets to the array, the parser already parsed and resolved the size).

pal/src/host/linux-sgx/pal_devices.c line 335 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

or a special "pointer" sub-region

Stale comment? There's no such value in this enum.

Done.

pal/src/host/linux-sgx/pal_devices.c line 344 at r20 (raw file):

struct mem_region {
    toml_array_t* toml_mem_region; /* describes contigious sub_regions in this mem_region */

Done.

pal/src/host/linux-sgx/pal_devices.c line 344 at r20 (raw file):

struct mem_region {
    toml_array_t* toml_mem_region; /* describes contigious sub_regions in this mem_region */

Done.

pal/src/host/linux-sgx/pal_devices.c line 401 at r20 (raw file):

        }
    }
    return -PAL_ERROR_NOTDEFINED;

FYI: NOTDEFINED PAL error is translated into ENOSYS in LibOS, and this looks very confusing. So I changed to more standard INVAL.

pal/src/host/linux-sgx/pal_devices.c line 440 at r20 (raw file):

    int64_t align;
    int ret = toml_int_in(toml_sub_region, "align", /*defaultval=*/0, &align);
    if (ret < 0 || align < 0)

Why? align == 0 means "no need for alignment". Isn't it valid?

pal/src/host/linux-sgx/pal_devices.c line 462 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

The current name was quite confusing for me, I had to read its code to get what it actually does.

Done.

pal/src/host/linux-sgx/pal_devices.c line 479 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Wouldn't it be cleaner (and faster) to pass the corresponding TOML object as an argument to this function? (instead of searching for it again)

I considered this, but if you look at the current code, there are too many level of functions. So we'd need to drag this toml_ioctl_structs object through a bunch of functions, just for this single usage. I don't find it "clean".

I could introduce some kind of the ioctl_parsing_state struct which would contain all these helpers and be populated across these functions -- but this also sounds like overkill (just for a single toml_ioctl_structs?).

Also note that this PR intentionally doesn't introduce any performance optimizations. This will be a follow-up work (see PR description).

pal/src/host/linux-sgx/pal_devices.c line 517 at r20 (raw file):

    }

    /* size must be specified as string (another sub-region's name) */

Done.

pal/src/host/linux-sgx/pal_devices.c line 531 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't like this implicit pointer conversion (copy_value() expects uint64_t*, but gets size_t* here). I'd rather read to uint64_t and then convert the value to size_t + a static assert for sizes.

Done.

pal/src/host/linux-sgx/pal_devices.c line 588 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

BOF if max_mem_regions == 0 (I know that in the current code it can't happen, but it's still a bad and dangerous practice)

Done. What does BOF mean?

pal/src/host/linux-sgx/pal_devices.c line 595 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This sentence would be more useful if it said why you have to use BFS. Now it's only "I have to use BFS despite [...]".

Done. Does this explanation help?

pal/src/host/linux-sgx/pal_devices.c line 770 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Is this a bug? The value of this condition can't change between iterations...

No, it's not a bug. To be honest, I don't understand your problem.

Here we're protecting against the following case:

    struct ioctl_blabla {
        char* arr_of_bufs[10];
    }

    struct ioctl_blabla x;
    x.arr_of_bufs[0] = 0x42; // points to mem region, must also parse it
    x.arr_of_bufs[1] = NULL; // this doesn't point to any mem region, so must be skipped
    x.arr_of_bufs[2] = 0x24; // points to mem region, must also parse it

pal/src/host/linux-sgx/pal_devices.c line 789 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

sub_regions will be used afterwards by the caller, so please zero all freed pointers (here and above).

Done. Not sure I understand what exactly you want, but hopefully this.

pal/src/host/linux-sgx/pal_devices.c line 794 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Everything here and below needs fixing against ÆPIC Leak, we can't just memset/memcpy memory in and out, unfortunately.
Note to myself: continue reviewing from this point once this is fixed.

I think I fixed everything against ÆPIC Leak.

Why do you say that memset(some-untrusted-region, 0) is not secure?

kailun-qin

Reviewable status: 14 of 26 files reviewed, 66 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

Documentation/manifest-syntax.rst line 839 at r16 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean? The key is called align. Do you suggest to rename the key?

Looks reasonable since the other fields are nouns.

Documentation/manifest-syntax.rst line 838 at r21 (raw file):

- ``name`` is an optional name for this sub-region; mainly used to find
  length-specifying fields and nested memory regions.

sub-regions? (we should try to not mix the term field and sub-region)

Code quote:

fields

Documentation/manifest-syntax.rst line 843 at r21 (raw file):

  contiguous with the first sub-region, so specifying their alignment doesn't
  make sense).
- ``size`` is a size of this sub-region. The ``size`` field may be a string with

the

Code quote:

Documentation/manifest-syntax.rst line 844 at r21 (raw file):

  make sense).
- ``size`` is a size of this sub-region. The ``size`` field may be a string with
  the name of another field that contains the size value or an integer with the

sub-region?

Code quote:

field

Documentation/manifest-syntax.rst line 859 at r21 (raw file):

  results in a total size of 4B.
- ``array_len`` denotes the number of items in the ``ptr`` array. This field
  cannot be specified with non-``ptr`` regions.

Is it better that we move this after ptr? Also, we should state that it's optional and its default vaule (as the other fields do).

Code quote:

- ``array_len`` denotes the number of items in the ``ptr`` array. This field
  cannot be specified with non-``ptr`` regions.

Documentation/manifest-syntax.rst line 859 at r21 (raw file):

  results in a total size of 4B.
- ``array_len`` denotes the number of items in the ``ptr`` array. This field
  cannot be specified with non-``ptr`` regions.

sub-regions

Code quote:

regions

Documentation/manifest-syntax.rst line 932 at r21 (raw file):

   insecure by themselves in SGX environments:

       - IOCTL arguments are passed as-is from the app to the untrusted host,

This renders a bit wrong. Or you intended this way?

libos/src/sys/libos_ioctl.c line 48 at r21 (raw file):

    unlock(&g_dcache_lock);

    if (is_host_dev) {

Any special reason that we invoke this PAL call before sanity checks (in the SWITCH below)?

pal/src/host/linux-sgx/pal_devices.c line 440 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why? align == 0 means "no need for alignment". Isn't it valid?

Shouldn't it alignment be a power of two (i.e., 0 is invalid)?

pal/src/host/linux-sgx/pal_devices.c line 794 at r20 (raw file):

Why do you say that memset(some-untrusted-region, 0) is not secure?

Intel-SA-00615 (all writes to untrusted memory from within the enclave must be done in 8-byte chunks aligned to 8-bytes boundary)?

pal/src/host/linux-sgx/pal_devices.c line 252 at r21 (raw file):

 * IOCTL could for example be used to convert a Pascal string into a C string (C string will be
 * truncated to user-specified `s2_size` if greater than this limit), and find the indices of the
 * first occurences of chars "x" and "y" in the Pascal string.

occurrences

Code quote:

occurences

pal/src/host/linux-sgx/pal_devices.c line 266 at r21 (raw file):

 *     { name = "c-str-size", size = 8, direction = "inout" },
 *     { size = 1, direction = "in" }
 *     { size = 1, direction = "in" }

So it's two SRs or one SR?

Code quote:

 *     { size = 1, direction = "in" }
 *     { size = 1, direction = "in" }

pal/src/host/linux-sgx/pal_devices.c line 282 at r21 (raw file):

 *     and doesn't have an in/out direction. The `array_len` field specifies the number of adjacent
 *     memory regions that this pointer points to (i.e. the length of the array).
 *  4. Sub-regions can be fixed-size (like the last sub-region containing two bytes `x` and `y`) or

So it's two SRs or one SR? same question as above

Code quote:

like the last sub-region containing two bytes `x` and `y`

pal/src/host/linux-sgx/pal_devices.c line 285 at r21 (raw file):

 *     can be flexible-size (like the two strings). In the latter case, the `size` field contains a
 *     name of a sub-region where the actual size is stored. Note that this referenced sub-region
 *     must come *before* the sub-region with such flexible-size `size` -- TOML representations of

This seems to be contradicting w/ the example above?

       { ptr = [
           { name = "c-str", size = "c-str-size", direction = "in" }
         ] },
       { name = "c-str-size", size = 8, direction = "inout" },

Code quote:

 *     name of a sub-region where the actual size is stored. Note that this referenced sub-region
 *     must come *before* the sub-region with such flexible-size `size` -- TOML representations of

pal/src/host/linux-sgx/pal_devices.c line 316 at r21 (raw file):

 *  |   |  uint64_t s2_len | |   SR3    |    |  |   uint64_t s2_len      |  |
 *  |   |                  | |          |    |  |                        |  |
 *  |   |  int8_t x, y     | |   SR4    |    |  |   int8_t x, y          |  |

So it's two SRs or one SR? same question as above

Code quote:

int8_t x, y     | |   SR4

pal/src/host/linux-sgx/pal_devices.c line 396 at r21 (raw file):

                              const char* sub_region_name, size_t* out_idx) {
    /* it is important to iterate in reverse order because there may be an array of mem regions
     * with same-named sub regions, and we want to find the latest sub region */

Should we doc this somewhere e.g., in the limitations above?

Code quote:

    /* it is important to iterate in reverse order because there may be an array of mem regions
     * with same-named sub regions, and we want to find the latest sub region */

pal/src/host/linux-sgx/pal_devices.c line 443 at r21 (raw file):

static int get_sub_region_align(const toml_table_t* toml_sub_region, size_t* out_align) {
    int64_t align;
    int ret = toml_int_in(toml_sub_region, "align", /*defaultval=*/0, &align);

8?

Code quote:

/*defaultval=*/0

pal/src/host/linux-sgx/pal_devices.c line 481 at r21 (raw file):

    if (!ioctl_struct_str) {
        *out_toml_mem_region = NULL;
        return 0;

Why do we return 0 in such case?

Code quote:

return 0;

pal/src/host/linux-sgx/pal_devices.c line 614 at r21 (raw file):

     * sub-region's "name". Consider this example (unnecessary fields not shown for simplicity):
     *
     *   ioctl_read = [ { ptr = [ { size = "buf_size" } ] }, { name = "buf_size" } ]

hmm.. but we have the above note - Note that this referenced sub-region must come *before* the sub-region with such flexible-size size``?

Code quote:

ioctl_read = [ { ptr = [ { size = "buf_size" } ] }, { name = "buf_size" } ]

pal/src/host/linux-sgx/pal_devices.c line 716 at r21 (raw file):

            }

            ret = get_sub_region_array_len(toml_sub_region, &cur_sub_region->array_len);

Shouldn't this field only make sense for non-ptr sub-regions?

Code quote:

ret = get_sub_region_array_len(toml_sub_region, &cur_sub_region->array_len);

pal/src/host/linux-sgx/pal_devices.c line 781 at r21 (raw file):

                }

                free(sub_regions[i].array_len.sub_region_name);

This seems to be unneeded.

Code quote:

free(sub_regions[i].array_len.sub_region_name);

pal/src/host/linux-sgx/pal_devices.c line 953 at r21 (raw file):

    ssize_t toml_allowed_ioctls_cnt = toml_array_nelem(toml_allowed_ioctls);
    if (toml_allowed_ioctls_cnt <= 0)
        return -PAL_ERROR_NOTIMPLEMENTED;

Is this right that in this function, we default to return -PAL_ERROR_NOTIMPLEMENTED in case of errors.
Note to myself: continue reviewing from this point

Code quote:

PAL_ERROR_NOTIMPLEMENTED

kailun-qin

Reviewable status: 14 of 26 files reviewed, 66 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

a discussion (no related file):

could you also take a look at this PR?

@mkow I did a first pass on the PR and made some comments/questions. I'll finish the rest and follow up all discussions.

dimakuv

Reviewable status: 14 of 26 files reviewed, 66 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @kailun-qin, and @mkow)

Documentation/manifest-syntax.rst line 839 at r16 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Looks reasonable since the other fields are nouns.

Done.

Documentation/manifest-syntax.rst line 854 at r16 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

What do you mean? The key is called adjust. Do you suggest to rename the key?

Done. Renamed the key.

Documentation/manifest-syntax.rst line 838 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

sub-regions? (we should try to not mix the term field and sub-region)

Done.

Documentation/manifest-syntax.rst line 843 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

the

Done.

Documentation/manifest-syntax.rst line 844 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

sub-region?

Done.

Documentation/manifest-syntax.rst line 859 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Is it better that we move this after ptr? Also, we should state that it's optional and its default vaule (as the other fields do).

Done.

Documentation/manifest-syntax.rst line 859 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

sub-regions

Done.

Documentation/manifest-syntax.rst line 932 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

This renders a bit wrong. Or you intended this way?

Done. Good catch.

libos/src/sys/libos_ioctl.c line 48 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Any special reason that we invoke this PAL call before sanity checks (in the SWITCH below)?

The only sanity check that we have below is the is_user_memory_writable(arg) one. But this is not applicable here -- some IOCTLs do not specify the arg at all, some IOCTLs have it read-only, some IOCTLs have it write-only. We can't know how to check arg in a general case. That's why I didn't add any checks.

pal/src/host/linux-sgx/pal_devices.c line 440 at r20 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Shouldn't it alignment be a power of two (i.e., 0 is invalid)?

Yeah, ok, align==0 doesn't make sense. Done.

pal/src/host/linux-sgx/pal_devices.c line 794 at r20 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why do you say that memset(some-untrusted-region, 0) is not secure?

Intel-SA-00615 (all writes to untrusted memory from within the enclave must be done in 8-byte chunks aligned to 8-bytes boundary)?

Done. True, even if we write all-zeros, we are still susceptible to this vulnerability.

I removed all memset()s since they were anyway only for sanity. Plus added a quick comment.

pal/src/host/linux-sgx/pal_devices.c line 252 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

occurrences

Done.

pal/src/host/linux-sgx/pal_devices.c line 266 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

So it's two SRs or one SR?

Done. Right, I had a mismatch in my descriptions. It is one SR.

pal/src/host/linux-sgx/pal_devices.c line 282 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

So it's two SRs or one SR? same question as above

Done.

pal/src/host/linux-sgx/pal_devices.c line 285 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

This seems to be contradicting w/ the example above?

       { ptr = [
           { name = "c-str", size = "c-str-size", direction = "in" }
         ] },
       { name = "c-str-size", size = 8, direction = "inout" },

No, I don't think there is any contradiction. See my other reply on a similar question.

pal/src/host/linux-sgx/pal_devices.c line 316 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

So it's two SRs or one SR? same question as above

Done. One SR.

pal/src/host/linux-sgx/pal_devices.c line 396 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Should we doc this somewhere e.g., in the limitations above?

I don't think we want to document it, as I have never seen a real-life IOCTL that would specify sub-regions in incorrect order.

pal/src/host/linux-sgx/pal_devices.c line 443 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

8?

Why 8? But you're right, 0 didn't make sense. I modified the default value to 1.

pal/src/host/linux-sgx/pal_devices.c line 481 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why do we return 0 in such case?

This means that there's no ptr field at all in this sub-region (recall that toml_string_in() returns a NULL string if it doesn't find the key). So it's a success -- in the sense that there is no nested memory region (i.e., the current sub-region is a non-ptr sub-region).

pal/src/host/linux-sgx/pal_devices.c line 614 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

hmm.. but we have the above note - Note that this referenced sub-region must come *before* the sub-region with such flexible-size size``?

"Before" means in the BFS order. In this example, the { name = "buf_size" } sub-region is located in the outer memory region, and the { size = "buf_size" } sub-region (which references the former sub-region) is located in the inner memory region.

Thus, { name = "buf_size" } comes before { size = "buf_size" }.

I added a quick comment in that Note that this ... sentence.

pal/src/host/linux-sgx/pal_devices.c line 716 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Shouldn't this field only make sense for non-ptr sub-regions?

Was it a typo? Did you mean "the array_len field only makes sense together with the ptr field"?

I can add such a check, if that's what you wanted. Done.

pal/src/host/linux-sgx/pal_devices.c line 781 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

This seems to be unneeded.

Why? It is needed, because we overwrite sub_regions[i].array_len object immediately afterwards (and thus we would lose the reference to the previously allocated sub_regions[i].array_len.sub_region_name).

pal/src/host/linux-sgx/pal_devices.c line 953 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Is this right that in this function, we default to return -PAL_ERROR_NOTIMPLEMENTED in case of errors.
Note to myself: continue reviewing from this point

I'd say yes. This covers the case like this: sgx.allowed_ioctls = [ ] (empty set). In this case, it looks reasonable to me to return -ENOSYS ("I don't know this IOCTL") rather than -EINVAL ("I couldn't parse this IOCTL correctly").

mkow

Reviewed 6 of 12 files at r21, 6 of 6 files at r22, all commit messages.
Reviewable status: all files reviewed, 35 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @kailun-qin)

Documentation/manifest-syntax.rst line 839 at r16 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Yes, as Kailun said, almost all others are nouns (and a verb doesn't make much sense as a struct field name).

libos/test/regression/device_ioctl_parse_fail.manifest.template line 33 at r22 (raw file):

]

# Buffer is defined before its size is defined (parser doesn't "know" when buffer ends and fails)

Suggestion:

parser doesn't "know" where buffer ends and fails

pal/src/host/linux-sgx/pal_devices.c line 244 at r20 (raw file):
Maybe s2_buf_size? Or s2_alloc_size? It still sounds like it was the length of the passed string, but this time counting the NULL byte.
And yes, this example is really contrived, that's why I want to make the naming/description better - I had troubles understanding what's happening here.

and its actual size

This still sounds like you're talking about the size of the string, not the allocation size of the buffer holding it.

pal/src/host/linux-sgx/pal_devices.c line 287 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Though maybe this comment is not needed, because I think there are no legitimate cases where it would be possible, see below.

a bit harder to parse (I guess) if the size is located after the pointer?

I'm not sure I understand you. The size after the pointer is not a problem -- the pointer is 8-byte-sized always, and the array this pointer points to is always a nested memory region, so it will always be "after" the size, because we do Breadth First Search (in other words, when the parser gets to the array, the parser already parsed and resolved the size).

The BFS order was what was missing, I thought you're talking about top-down order (probably the default in standard English text).

pal/src/host/linux-sgx/pal_devices.c line 440 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yeah, ok, align==0 doesn't make sense. Done.

Yeah, "alignment" means what the address is divisible by, so zero doesn't make sense.

pal/src/host/linux-sgx/pal_devices.c line 479 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I considered this, but if you look at the current code, there are too many level of functions. So we'd need to drag this toml_ioctl_structs object through a bunch of functions, just for this single usage. I don't find it "clean".

I could introduce some kind of the ioctl_parsing_state struct which would contain all these helpers and be populated across these functions -- but this also sounds like overkill (just for a single toml_ioctl_structs?).

Also note that this PR intentionally doesn't introduce any performance optimizations. This will be a follow-up work (see PR description).

Ok, let's keep it as it is then.

pal/src/host/linux-sgx/pal_devices.c line 588 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. What does BOF mean?

Buffer OverFlow

pal/src/host/linux-sgx/pal_devices.c line 770 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, it's not a bug. To be honest, I don't understand your problem.

Here we're protecting against the following case:

    struct ioctl_blabla {
        char* arr_of_bufs[10];
    }

    struct ioctl_blabla x;
    x.arr_of_bufs[0] = 0x42; // points to mem region, must also parse it
    x.arr_of_bufs[1] = NULL; // this doesn't point to any mem region, so must be skipped
    x.arr_of_bufs[2] = 0x24; // points to mem region, must also parse it

This check is either true for all iterations or always false for all of them. It doesn't depend on k or any other loop-related variable, that's why it looks suspicious, especially that you need some calculations to check it. Is this intended? If so, then it's a rather weird construct... Why not move it out of the loop?

Actually, this loop does exactly nothing if the check is false, shouldn't it cover the whole loop then? I.e.:

void* mem_region_addr = *((void**)sub_regions[i].enclave_addr);
if (mem_region_addr) {
    for (size_t k = 0; k < sub_regions[i].array_len.value; k++) {
        [...]
    }
}

pal/src/host/linux-sgx/pal_devices.c line 789 at r20 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Not sure I understand what exactly you want, but hopefully this.

Yes, this. Otherwise we'll end up with dangling pointers saved there, which are easy to accidentally misuse.

pal/src/host/linux-sgx/pal_devices.c line 680 at r22 (raw file):

                }
            } else {
                if (toml_raw_in(toml_sub_region, "array_len")) {

Please fold to } else if (...) {.

pal/src/host/linux-sgx/pal_devices.c line 835 at r22 (raw file):

                                         void* untrusted_addr) {
    /* we rely on the fact that the untrusted memory region was zeroed out: we can simply "jump
     * over" untrusted memory when doing alignment and when direction of copy is in or none */

Suggestion:

direction of copy is `in` or `none`

pal/src/host/linux-sgx/pal_devices.c line 914 at r22 (raw file):

    if (ret < 0) {
        ret = -PAL_ERROR_INVAL;
        goto out;

This seems wrong: toml_rtos() failed, but you still call free() on its result (ioctl_struct_str).

pal/src/host/linux-sgx/pal_devices.c line 1031 at r22 (raw file):

    for (size_t i = 0; i < sub_regions_cnt; i++) {
        /* overapproximation since alignment doesn't necessarily increase sub-region's size */
        untrusted_size += sub_regions[i].size + sub_regions[i].alignment;

Suggestion:

untrusted_size += sub_regions[i].size + sub_regions[i].alignment - 1;

kailun-qin

Reviewed 1 of 11 files at r6, 6 of 16 files at r14, 3 of 9 files at r16, 1 of 5 files at r17, 6 of 12 files at r21, 6 of 6 files at r22, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

libos/src/sys/libos_ioctl.c line 48 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The only sanity check that we have below is the is_user_memory_writable(arg) one. But this is not applicable here -- some IOCTLs do not specify the arg at all, some IOCTLs have it read-only, some IOCTLs have it write-only. We can't know how to check arg in a general case. That's why I didn't add any checks.

Make sense, thanks for the explanation!

pal/src/host/linux-sgx/pal_devices.c line 396 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't think we want to document it, as I have never seen a real-life IOCTL that would specify sub-regions in incorrect order.

Fair enough.

pal/src/host/linux-sgx/pal_devices.c line 443 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why 8? But you're right, 0 didn't make sense. I modified the default value to 1.

Just giving an example, I'm fine w/ 1.

pal/src/host/linux-sgx/pal_devices.c line 481 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This means that there's no ptr field at all in this sub-region (recall that toml_string_in() returns a NULL string if it doesn't find the key). So it's a success -- in the sense that there is no nested memory region (i.e., the current sub-region is a non-ptr sub-region).

Got it, thanks!

pal/src/host/linux-sgx/pal_devices.c line 614 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

"Before" means in the BFS order. In this example, the { name = "buf_size" } sub-region is located in the outer memory region, and the { size = "buf_size" } sub-region (which references the former sub-region) is located in the inner memory region.

Thus, { name = "buf_size" } comes before { size = "buf_size" }.

I added a quick comment in that Note that this ... sentence.

Thanks for the clarification for BFS order. Looks good to me now.

pal/src/host/linux-sgx/pal_devices.c line 716 at r21 (raw file):

Was it a typo?

Ah, it was a typo.

Did you mean "the array_len field only makes sense together with the ptr field"?

Yes

I can add such a check, if that's what you wanted. Done.

Thanks!

pal/src/host/linux-sgx/pal_devices.c line 781 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why? It is needed, because we overwrite sub_regions[i].array_len object immediately afterwards (and thus we would lose the reference to the previously allocated sub_regions[i].array_len.sub_region_name).

Yes, you are right -- I misread it.

pal/src/host/linux-sgx/pal_devices.c line 953 at r21 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'd say yes. This covers the case like this: sgx.allowed_ioctls = [ ] (empty set). In this case, it looks reasonable to me to return -ENOSYS ("I don't know this IOCTL") rather than -EINVAL ("I couldn't parse this IOCTL correctly").

OK, make sense!

pal/src/host/linux-sgx/pal_devices.c line 310 at r22 (raw file):

 *      struct root (MR1)                   |       deep-copied struct (aligned at 128B)
 *      +----------------------+            |       +-----------------------------+
 *  +----+ pascal_str* s1      |     SR1    |    +----+ pascal_str* s1    (MR1)   |

nit: align the text right

+----+ pascal_str* s1      (MR1) |

Code quote:

+----+ pascal_str* s1    (MR1)   |

pal/src/host/linux-sgx/pal_devices.c line 853 at r22 (raw file):

            bool ret = sgx_copy_from_enclave(cur_untrusted_addr, sub_regions[i].enclave_addr,
                                             sub_regions[i].size);
            if (!ret)

Why not simply

if (!sgx_from_to_enclave())
    return ...

Code quote:

            bool ret = sgx_copy_from_enclave(cur_untrusted_addr, sub_regions[i].enclave_addr,
                                             sub_regions[i].size);
            if (!ret)

pal/src/host/linux-sgx/pal_devices.c line 874 at r22 (raw file):

                                                     sizeof(void*));
                    if (!ret)
                        return -PAL_ERROR_DENIED;

ditto

Code quote:

                    bool ret = sgx_copy_from_enclave(sub_regions[i].untrusted_addr,
                                                     &sub_regions[j].untrusted_addr,
                                                     sizeof(void*));
                    if (!ret)
                        return -PAL_ERROR_DENIED;

pal/src/host/linux-sgx/pal_devices.c line 893 at r22 (raw file):

            bool ret = sgx_copy_to_enclave(sub_regions[i].enclave_addr, sub_regions[i].size,
                                           sub_regions[i].untrusted_addr, sub_regions[i].size);
            if (!ret)

Why not simply

if (!sgx_copy_to_enclave())
    return ...

Code quote:

            bool ret = sgx_copy_to_enclave(sub_regions[i].enclave_addr, sub_regions[i].size,
                                           sub_regions[i].untrusted_addr, sub_regions[i].size);
            if (!ret)

dimakuv

Reviewable status: 24 of 26 files reviewed, 18 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @kailun-qin, and @mkow)

libos/test/regression/device_ioctl_parse_fail.manifest.template line 33 at r22 (raw file):

]

# Buffer is defined before its size is defined (parser doesn't "know" when buffer ends and fails)

Done.

pal/src/host/linux-sgx/pal_devices.c line 244 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Maybe s2_buf_size? Or s2_alloc_size? It still sounds like it was the length of the passed string, but this time counting the NULL byte.
And yes, this example is really contrived, that's why I want to make the naming/description better - I had troubles understanding what's happening here.

and its actual size

This still sounds like you're talking about the size of the string, not the allocation size of the buffer holding it.

Done.

pal/src/host/linux-sgx/pal_devices.c line 770 at r20 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This check is either true for all iterations or always false for all of them. It doesn't depend on k or any other loop-related variable, that's why it looks suspicious, especially that you need some calculations to check it. Is this intended? If so, then it's a rather weird construct... Why not move it out of the loop?

Actually, this loop does exactly nothing if the check is false, shouldn't it cover the whole loop then? I.e.:
void* mem_region_addr = *((void**)sub_regions[i].enclave_addr);
if (mem_region_addr) {
    for (size_t k = 0; k < sub_regions[i].array_len.value; k++) {
        [...]
    }
}

Done. Sorry, I thought that by "loop" and "iterations" you mean the outer while loop. But you mean the inner for loop. Yes, you are right. Fixed according to your proposal.

pal/src/host/linux-sgx/pal_devices.c line 680 at r22 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please fold to } else if (...) {.

Done.

pal/src/host/linux-sgx/pal_devices.c line 835 at r22 (raw file):

                                         void* untrusted_addr) {
    /* we rely on the fact that the untrusted memory region was zeroed out: we can simply "jump
     * over" untrusted memory when doing alignment and when direction of copy is in or none */

Done.

pal/src/host/linux-sgx/pal_devices.c line 853 at r22 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why not simply

if (!sgx_from_to_enclave())
    return ...

In my opinion, your proposed construct obscures the intention here -- I want to make it very explicit that it this point, we make the actual copy. I would prefer to keep the code as is. It looks weird, but that's exactly my intention -- the reader should "stop" on this line and think what happens here.

pal/src/host/linux-sgx/pal_devices.c line 874 at r22 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

ditto

ditto (I want it written in this weird very explicit way)

pal/src/host/linux-sgx/pal_devices.c line 893 at r22 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Why not simply

if (!sgx_copy_to_enclave())
    return ...

ditto (I want it written in this weird very explicit way)

pal/src/host/linux-sgx/pal_devices.c line 914 at r22 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This seems wrong: toml_rtos() failed, but you still call free() on its result (ioctl_struct_str).

Done. Though it wasn't wrong -- if toml_rtos() fails then it doesn't modify ioctl_struct_str which was set to NULL beforehand. So free() would be just a no-op.

pal/src/host/linux-sgx/pal_devices.c line 1031 at r22 (raw file):

    for (size_t i = 0; i < sub_regions_cnt; i++) {
        /* overapproximation since alignment doesn't necessarily increase sub-region's size */
        untrusted_size += sub_regions[i].size + sub_regions[i].alignment;

Done.

kailun-qin

Reviewed 2 of 2 files at r23, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

pal/src/host/linux-sgx/pal_devices.c line 853 at r22 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

In my opinion, your proposed construct obscures the intention here -- I want to make it very explicit that it this point, we make the actual copy. I would prefer to keep the code as is. It looks weird, but that's exactly my intention -- the reader should "stop" on this line and think what happens here.

OK I see. Thanks for the explanation!

kailun-qin

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

libos/test/regression/device_ioctl_parse_fail.manifest.template line 33 at r22 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Not done?

kailun-qin

Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

libos/test/regression/device_ioctl_parse_fail.manifest.template line 33 at r22 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Not done?

Update: Oops, its done, excuse me for my blindness

kailun-qin

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

pal/src/host/linux-sgx/pal_devices.c line 716 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

Was it a typo?

Ah, it was a typo.

Did you mean "the array_len field only makes sense together with the ptr field"?

Yes

I can add such a check, if that's what you wanted. Done.

Thanks!

I re-read this. The added check only ensures that non-ptr field cannot specify array_len. But since we're still calling get_sub_region_array_len() here so we'll always have a default array_len assigned to the current sub-region?

dimakuv

Reviewable status: 25 of 26 files reviewed, 16 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @kailun-qin, and @mkow)

pal/src/host/linux-sgx/pal_devices.c line 716 at r21 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

I re-read this. The added check only ensures that non-ptr field cannot specify array_len. But since we're still calling get_sub_region_array_len() here so we'll always have a default array_len assigned to the current sub-region?

Done. You are correct. This is a bit weird. I added a comment explaining that this is a benign (though unnecessary) invocation. I think the comment is easier than adding IF checks here.

kailun-qin

Reviewable status: 25 of 26 files reviewed, 15 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)

mkow

Reviewed 1 of 2 files at r23, 1 of 1 files at r24, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

By the way, an interesting consequence of current dev files implementation in Gramine.

We typically write like this (see example in this PR), to enable a device inside Gramine:
fs.mounts = [
  { path = "/dev/gramine_test_dev", uri = "dev:/dev/gramine_test_dev" },
]
Sometimes, however, the device name is not known in advance (some devices like to have suffixes like /dev/tty42), and this name differs from one machine to the other. Currently, Gramine's FS code doesn't allow to open any device; the device name must be hard-coded in the manifest as above.

I am not sure we want to change this behavior... I see one workaround for such a problem: request Gramine deployers to prepare their machines -- to create symlinks from the actual device name to the hard-coded name in the manifest. E.g. the deployers must perform smth like ln -s /dev/actual_device_name_42 /dev/gramine_test_dev on each machine.

I wouldn't change this, it makes dangerous configuration of Gramine easier than the safe one.

pal/src/host/linux-sgx/pal_devices.c line 914 at r22 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Though it wasn't wrong -- if toml_rtos() fails then it doesn't modify ioctl_struct_str which was set to NULL beforehand. So free() would be just a no-op.

That's what we do as a convention in Gramine, not sure if other libraries are also that sane ;)

Borys asked to dismiss him, since he's not working on this project anymore.

dimakuv

Dismissed @boryspoplawski from 5 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners

dimakuv

Reviewable status: 17 of 26 files reviewed, 2 unresolved discussions (waiting on @kailun-qin and @mkow)

-- commits line 27 at r16:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

TODO: And on the third test.

Done

-- commits line 27 at r16:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do at final rebase.

Done

kailun-qin

Reviewed 1 of 1 files at r24, 9 of 9 files at r25, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dimakuv and @mkow)

-- commits line 30 at r25:
-> IOCTL structs maybe? The third test is not exactly checking on the manifest "formatting" IMO.

Code quote:

manifest formatting

libos/test/regression/device_ioctl_parse_fail.c line 1 at r25 (raw file):

#define _GNU_SOURCE /* for loff_t */

is this needed?

Code quote:

#define _GNU_SOURCE /* for loff_t */

libos/test/regression/device_ioctl_parse_fail.c line 5 at r25 (raw file):

#include <errno.h>
#include <fcntl.h>
#include <stdint.h>

ditto

Code quote:

#include <stdint.h>

libos/test/regression/device_ioctl_parse_fail.c line 7 at r25 (raw file):

#include <stdint.h>
#include <stdio.h>
#include <string.h>

ditto

Code quote:

#include <string.h>

The newly added `ioctl()` syscall emulation on device-backed file descriptors is pass-through. It is insecure by itself since the emulation only passes the arguments to and from the untrusted memory: - IOCTL arguments are passed as-is from the app to the untrusted host, which may lead to leaks of secret data; - untrusted host can change IOCTL arguments as it wishes when passing them from Gramine to the device and back. It is the responsibility of the app developer to correctly use IOCTLs, with security implications in mind. In most cases, IOCTL arguments should be encrypted or integrity-protected with a key pre-shared between Gramine and the device. On the Linux-SGX PAL, a set of IOCTL requests must be explicitly allowed in the manifest via the new option `sgx.allowed_ioctls`. Also, the allowed IOCTLs' arguments (typically pointers to complex nested objects) must be explicitly described in the manifest via the new option `sgx.ioctl_structs.[identifier]` and a corresponding reference in `sgx.allowed_ioctls`; see docs for explanation of the IOCTL struct format. This commit adds three new LibOS tests to verify the flexible IOCTL logic against Gramine dummy device `/dev/gramine_test_dev`. This device is located in companion repo `gramineproject/device-testing-tools`. One test checks IOCTL data passing, second test checks that Gramine forbids unknown IOCTLs, third test checks that Gramine IOCTL parser fails on incorrectly defined IOCTL structs. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

dimakuv

Reviewable status: 25 of 26 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)

-- commits line 30 at r25:

Previously, kailun-qin (Kailun Qin) wrote…

-> IOCTL structs maybe? The third test is not exactly checking on the manifest "formatting" IMO.

Done.

libos/test/regression/device_ioctl_parse_fail.c line 1 at r25 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

is this needed?

Yes, required, otherwise:

In file included from ../libos/test/regression/device_ioctl_parse_fail.c:12:
../libos/test/regression/gramine_test_dev_ioctl.h:16:5: error: unknown type name ‘loff_t’

libos/test/regression/device_ioctl_parse_fail.c line 5 at r25 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

ditto

Done.

libos/test/regression/device_ioctl_parse_fail.c line 7 at r25 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

ditto

Done.

kailun-qin

Reviewed 1 of 1 files at r26, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

mkow

Reviewed 9 of 9 files at r25, 1 of 1 files at r26, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv marked this pull request as draft June 21, 2022 17:27

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch from cf9677e to c7f01d7 Compare June 22, 2022 14:26

dimakuv mentioned this pull request Jun 22, 2022

Add IOCTLs to the dummy Gramine device gramineproject/device-testing-tools#4

Merged

dimakuv mentioned this pull request Jul 15, 2022

RFC: Add POSIX shared untrusted memory support #757

Closed

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch 2 times, most recently from b0d7dea to 60c2c64 Compare July 25, 2022 12:34

dimakuv commented Jul 25, 2022

View reviewed changes

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch from 60c2c64 to c5d8165 Compare July 25, 2022 13:47

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch from c5d8165 to 9d2ac4b Compare October 13, 2022 11:37

dimakuv commented Oct 13, 2022

View reviewed changes

dimakuv mentioned this pull request Oct 19, 2022

toml Python package v0.10.0 is buggy; need v0.10.2 #990

Closed

boryspoplawski requested changes Oct 20, 2022

View reviewed changes

dimakuv mentioned this pull request Oct 20, 2022

Move from toml Python module to tomli #968

Closed

dimakuv commented Oct 24, 2022

View reviewed changes

boryspoplawski requested changes Oct 24, 2022

View reviewed changes

dimakuv commented Oct 25, 2022

View reviewed changes

boryspoplawski requested changes Oct 25, 2022

View reviewed changes

boryspoplawski requested changes Oct 27, 2022

View reviewed changes

dimakuv commented Oct 31, 2022

View reviewed changes

boryspoplawski requested changes Oct 31, 2022

View reviewed changes

boryspoplawski requested changes Nov 1, 2022

View reviewed changes

mkow requested changes Nov 1, 2022

View reviewed changes

boryspoplawski requested changes Nov 1, 2022

View reviewed changes

mkow reviewed Nov 2, 2022

View reviewed changes

boryspoplawski requested changes Nov 2, 2022

View reviewed changes

dimakuv commented Nov 3, 2022

View reviewed changes

dimakuv commented Nov 4, 2022

View reviewed changes

mkow reviewed May 10, 2023

View reviewed changes

mkow requested changes May 11, 2023

View reviewed changes

dimakuv commented May 12, 2023

View reviewed changes

kailun-qin requested changes May 15, 2023

View reviewed changes

kailun-qin reviewed May 15, 2023

View reviewed changes

dimakuv commented May 15, 2023

View reviewed changes

mkow requested changes May 18, 2023

View reviewed changes

kailun-qin requested changes May 19, 2023

View reviewed changes

dimakuv commented May 19, 2023

View reviewed changes

kailun-qin approved these changes May 19, 2023

View reviewed changes

kailun-qin requested changes May 19, 2023

View reviewed changes

kailun-qin approved these changes May 19, 2023

View reviewed changes

kailun-qin requested changes May 19, 2023

View reviewed changes

dimakuv commented May 19, 2023

View reviewed changes

kailun-qin approved these changes May 19, 2023

View reviewed changes

mkow previously approved these changes May 26, 2023

View reviewed changes

dimakuv commented May 29, 2023

View reviewed changes

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch from ed952fb to ba1eb85 Compare May 30, 2023 07:58

dimakuv commented May 30, 2023

View reviewed changes

kailun-qin requested changes May 30, 2023

View reviewed changes

dimakuv dismissed mkow’s stale review via e3ca202 May 30, 2023 09:33

dimakuv force-pushed the dimakuv/add-flexible-ioctl branch from ba1eb85 to e3ca202 Compare May 30, 2023 09:33

dimakuv commented May 30, 2023

View reviewed changes

kailun-qin approved these changes May 30, 2023

View reviewed changes

mkow approved these changes May 31, 2023

View reviewed changes

dimakuv merged commit e3ca202 into master May 31, 2023

dimakuv deleted the dimakuv/add-flexible-ioctl branch May 31, 2023 14:43

dimakuv mentioned this pull request Aug 1, 2023

[common] Add onlyif syntax to flexible IOCTLs #1482

Merged

[LibOS,PAL] Add flexible device-specific IOCTL support #671

[LibOS,PAL] Add flexible device-specific IOCTL support #671

Conversation

dimakuv commented Jun 21, 2022 • edited Loading

Description of the changes

How to test this PR?

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

boryspoplawski left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv commented Jun 21, 2022 •

edited

Loading