sync with upstreamed SGX in-kernel driver from mainline 5.11 #2084

johnvenn · 2021-01-14T07:42:33Z

Description of the changes

Intel SGX driver has been upstreamed to mainline from kernel version 5.11. To sync with upstreamed SGX in-kernel driver, as SGX device in the kernel is exposed to "/dev/sgx_enclave" instead of "/dev/sgx/enclave", graphene needs to support the upstreaned in-kernel SGX device node "dev/sgx_enclave" in link_intel_driver.py
Fixes /dev/sgx/enclave is gone in the latest in-tree SGX kernel #2007
Also we need to update documentation on sgx_intro.html for the upstreamed in-kernel SGX driver in linux mainline, exposed as /dev/sgx_enclave. And additional steps for user if they want the compatibility with OOT SGX driver exposed as /dev/sgx/enclave
For OOT drivers in DCAP from version 1.10, the header file is also changed, we need to sync with it. see:
Linux Driver: move use space header intel/SGXDataCenterAttestationPrimitives#155

To test this PR:

the 1st time rebooting to 5.11 linux kernel, check:
ls /dev | grep sgx ==> no /dev/sgx/enclave but /dev/sgx_enclave
Using upstreamed kernel 5.11, at Pal/src/host/Linux-SGX, run:
ISGX_DRIVER_PATH="/usr/src/linux-headers-$(uname -r)/arch/x86" ./link-intel-driver.py < ./gsgx.h.in > ./gsgx.h

A proper gsgx.h header will be generated, compilation pass.

This change is

dimakuv

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @johnvenn)

Documentation/sgx-intro.rst, line 93 at r1 (raw file):

  more "normal" PKI infrastructure).

- SGX driver is upstreamed to linux mainline from 5.11+ (see LKML patches).

There is a trailing space at the end of this line, please remove

Documentation/sgx-intro.rst, line 94 at r1 (raw file):

- SGX driver is upstreamed to linux mainline from 5.11+ (see LKML patches). 
  The current SGX driver upstreamed only supports DCAP. SGX device is exposed

There is a trailing space at the end of this line, please remove

Documentation/sgx-intro.rst, line 95 at r1 (raw file):

- SGX driver is upstreamed to linux mainline from 5.11+ (see LKML patches). 
  The current SGX driver upstreamed only supports DCAP. SGX device is exposed 
  as /dev/sgx_enclave at current stage, to make it compatible with older versions

I would prefer ; instead of ,: ...at current stage; to make it...

Documentation/sgx-intro.rst, line 96 at r1 (raw file):

  The current SGX driver upstreamed only supports DCAP. SGX device is exposed 
  as /dev/sgx_enclave at current stage, to make it compatible with older versions
  of SGX driver, user need to apply udev rules the first time rebooting the kernel.

user need to -> user must

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):

SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"

Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?

Pal/src/host/Linux-SGX/link-intel-driver.py, line 8 at r1 (raw file):

DRIVER_VERSIONS = {
        'sgx_user.h':                 '/dev/isgx',         # For Non-DCAP, older versions of legacy OOT SGX driver

There is a trailing space at the end of this line, please remove

Pal/src/host/Linux-SGX/link-intel-driver.py, line 9 at r1 (raw file):

DRIVER_VERSIONS = {
        'sgx_user.h':                 '/dev/isgx',
        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',

Please let's also keep this version (sgx_oot.h), to support DCAP drivers below 1.10

Pal/src/host/Linux-SGX/link-intel-driver.py, line 9 at r1 (raw file):

DRIVER_VERSIONS = {
        'sgx_user.h':                 '/dev/isgx',         # For Non-DCAP, older versions of legacy OOT SGX driver 
        'include/sgx_user.h':         '/dev/sgx/enclave',  # For Dcap driver from version 1.10+

For Dcap driver from version 1.10+ -> For DCAP driver version 1.10+

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r1 (raw file):

        'sgx_user.h':                 '/dev/isgx',
        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',
        'sgx_in_kernel.h':            '/dev/sgx/enclave',

Please let's also keep this version, to support the case where the user doesn't specify any SGX-driver directory (ISGX_DRIVER_PATH=""). And here the driver name must be changed to the new /dev/sgx_enclave.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 18 at r1 (raw file):

      - sgx_user.h for non-DCAP, older version of the driver
        (https://github.com/intel/linux-sgx-driver)
      - include/uapi/asm/sgx_oot.h for DCAP 1.6+ version of the driver

Please keep this, to support older versions of DCAP

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r1 (raw file):

      - include/uapi/asm/sgx_oot.h for DCAP 1.6+ version of the driver
        (https://github.com/intel/SGXDataCenterAttestationPrimitives)
      - default sgx_in_kernel.h for in-kernel 32+ version of the driver

Please keep this, to support the case where the user doesn't specify any SGX-driver directory (ISGX_DRIVER_PATH="").

Pal/src/host/Linux-SGX/link-intel-driver.py, line 59 at r1 (raw file):

    if not isgx_driver_path: #using in-kernel SGX driver from kernel version 5.11
        # user did not specify any driver path, use default in-kernel driver's C header
        # isgx_driver_path = os.path.dirname(os.path.abspath(__file__))

Why did you comment this line out?

Pal/src/host/Linux-SGX/link-intel-driver.py, line 67 at r1 (raw file):

                    raise FileNotFoundError
            except FileNotFoundError:
                print('SGX in-kernel driver upstreamed in mainline from kernel 5.11.\n'

was upstreamed in mainline kernel 5.11+.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 68 at r1 (raw file):

            except FileNotFoundError:
                print('SGX in-kernel driver upstreamed in mainline from kernel 5.11.\n'
                        'can\'t find SGX dev node /dev/sgx/enclave, '

Can not find the SGX dev node /dev/sgx/enclave, you must apply udev rules...

Pal/src/host/Linux-SGX/link-intel-driver.py, line 69 at r1 (raw file):

                print('SGX in-kernel driver upstreamed in mainline from kernel 5.11.\n'
                        'can\'t find SGX dev node /dev/sgx/enclave, '
                        'you need to apply udev rules to remap SGX devices by following steps:\n'

Just remove by following steps

Pal/src/host/Linux-SGX/link-intel-driver.py, line 71 at r1 (raw file):

                        'you need to apply udev rules to remap SGX devices by following steps:\n'
                        'sudo cp ${GRAPHENE_PROJECT_DIR}/Pal/src/host/Linux-SGX/91-sgx-enclave.rules /etc/udev/rules.d\n'
                        'sudo udevadm trigger',

Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?

Pal/src/host/Linux-SGX/link-intel-driver.py, line 74 at r1 (raw file):

                        file=sys.stderr)
                sys.exit(1)

Please remove spaces

dimakuv

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @johnvenn)

johnvenn · 2021-01-15T07:30:27Z

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):

SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?

Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?

[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

johnvenn · 2021-01-15T07:31:29Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r1 (raw file):

  - include/uapi/asm/sgx_oot.h for DCAP 1.6+ version of the driver
    (https://github.com/intel/SGXDataCenterAttestationPrimitives)
  - default sgx_in_kernel.h for in-kernel 32+ version of the driver

Please keep this, to support the case where the user doesn't specify any SGX-driver directory (ISGX_DRIVER_PATH="").

[Xiangping]: will update it in r3.

johnvenn · 2021-01-15T07:34:19Z

isgx_driver_path = os.path.dirname(os.path.abspath(file))

Why did you comment this line out?

[Xiangping]: sorry for the mistake, there are some problem if user uses in-kernel SGX driver header to compile graphene-SGX for this statement. I will update it to get sgx.h in kernel header

debin-yang · 2021-01-15T08:11:11Z

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):

SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?

Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?

[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

Just an opinion. I think it should not be the driver or its installer's role to do this (to provide compatibility support for other variants/versions of driver, except its own legacy versions of the same driver.). It should be abstracted higher level above driver.

Drivers (or SGX Linux kernel upstream patch) usually does not know of other variants of SGX Linux driver. This seems particularly true for the SGX Linux Upstream patches so far, along the way it kept changing its device node and/or interfaces, and it does not knows about its owns' previous versions, neither other SGX Linux drivers (SGX Linux OOT Driver, or SGX Linux DCAP drivers).

One reference design of solution for this issue, is the SGX Linux PSW's design which had been adapted to be compatible with all variants/legacy versions of SGX Linux kernel support, namely SGX Linux OOT Drivers, SGX DCAP Linux drivers (which had been tracking the SGX Linux Kernel Upstream patches' interfaces) /SGX Linux Kernel Upstream patches. So if following this reference, the solution (compatibility with various driver, and the upcoming SGX Linux kernel upstream patch) might go into somewhere into Graphene's installation process instead of the individual driver's installation.

debin-yang · 2021-01-15T08:26:00Z

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):
SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?
Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?
[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):
SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?
Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?
[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

Just an opinion. I think it should not be the driver or its installer's role to do this (to provide compatibility support for other variants/versions of driver, except its own legacy versions of the same driver.). It should be abstracted higher level above driver.

Drivers (or SGX Linux kernel upstream patch) usually does not know of other variants of SGX Linux driver. This seems particularly true for the SGX Linux Upstream patches so far, along the way it kept changing its device node and/or interfaces, and it does not knows about its owns' previous versions, neither other SGX Linux drivers (SGX Linux OOT Driver, or SGX Linux DCAP drivers).

One reference design of solution for this issue, is the SGX Linux PSW's design which had been adapted to be compatible with all variants/legacy versions of SGX Linux kernel support, namely SGX Linux OOT Drivers, SGX DCAP Linux drivers (which had been tracking the SGX Linux Kernel Upstream patches' interfaces) /SGX Linux Kernel Upstream patches. So if following this reference, the solution (compatibility with various driver, and the upcoming SGX Linux kernel upstream patch) might go into somewhere into Graphene's installation process instead of the individual driver's installati

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):
SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?
Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?
[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):
SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"
SUBSYSTEM=="sgx",KERNEL=="sgx/enclave",MODE="0666"
Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?
Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?
[Xiangping]: I agree with you, it is the driver installation procedure that should do this, but this step is not yet fixed in kernel. As an amendment on this transition, we need to provide instruction to users. This extra step can be omitted when in-kernel driver includes this step.

Just an opinion. I think it should not be the driver or its installer's role to do this (to provide compatibility support for other variants/versions of driver, except its own legacy versions of the same driver.). It should be abstracted higher level above driver.

Drivers (or SGX Linux kernel upstream patch) usually does not know of other variants of SGX Linux driver. This seems particularly true for the SGX Linux Upstream patches so far, along the way it kept changing its device node and/or interfaces, and it does not knows about its owns' previous versions, neither other SGX Linux drivers (SGX Linux OOT Driver, or SGX Linux DCAP drivers).

One reference design of solution for this issue, is the SGX Linux PSW's design which had been adapted to be compatible with all variants/legacy versions of SGX Linux kernel support, namely SGX Linux OOT Drivers, SGX DCAP Linux drivers (which had been tracking the SGX Linux Kernel Upstream patches' interfaces) /SGX Linux Kernel Upstream patches. So if following this reference, the solution (compatibility with various driver, and the upcoming SGX Linux kernel upstream patch) might go into somewhere into Graphene's installation process instead of the individual driver's installation.

To be more specific, currently the SGX Linux PSW's installer did this compatibility work. The individual drivers' installation does not know about their owns' previous implementation, or other variants of driver's implementation (including their earlier versions). And when release a new version of SGX PSW, we want the PSW to work fine on an existing SGX Linux platform which could have been installed with earlier variants of driver (could be SGX Linux OOT, SGX Linux DCAP, or SGX Linux kernel upstream patch candidates). This similar use case should apply to Graphene SGX too. (Courtesy to @hyjiang's advice on this.)

woju

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

Documentation/sgx-intro.rst, line 95 at r2 (raw file):

- SGX driver is upstreamed to linux mainline from 5.11+ (see LKML patches).
  The current SGX driver upstreamed only supports DCAP. SGX device is exposed
  as /dev/sgx_enclave at current stage; to make it compatible with older versions

NAK the idea of making the upstream driver compatible with older versions.
@debin-yang is right that this compat thingy should be done in our code.
Also oot and dcap drivers should adhere to whatever upstream driver does, in fact DCAP driver is maintained exactly this way, and OOT driver is binary-incompatible anyway, so we need to have compat adjustments anyway.

Documentation/sgx-intro.rst, line 98 at r2 (raw file):

  of SGX driver, user must apply udev rules the first time rebooting the kernel.
  For information in detail, please refer to:
  https://github.com/intel/SGXDataCenterAttestationPrimitives/blob/master/driver/linux/README.kernel.md#udev-rules

Reference to DCAP repo is wrong. Please write the install instructions here, and please refer to the .rules copy in our repo.

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 3 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why is this file needed? Isn't it the responsibility of the Intel SGX driver to do all these udev rules during installation?

AFAICT oot driver by default have the device at 666 (https://github.com/intel/linux-sgx-driver/blob/0373e2e8b96d9a261657b7657cb514f003f67094/sgx_main.c#L168) and dcap has this file: https://github.com/intel/SGXDataCenterAttestationPrimitives/blob/70f54dba09934381fad79bfe857f172e47819fc7/driver/linux/10-sgx.rules and instruction in README to install it. But the kernel driver has no such thing, because under kernel maintainers chmod 777 wouldn't fly and udev rules are maintained separately (https://github.com/systemd/systemd/tree/061e9fc5f1abc9d4a338a072803e293bd93c3c3e/rules.d). So this file was lifted from DCAP and, for lack of better idea, added here.

May I reiterate that this is distro business, not ours. The proper way would be to submit a PR against systemd/systemd after 5.11.0 release and wait for distros to carry the patch, but if we'd like to make user's life easier, we can have this in contrib/ and some reference in documentation (not in build system).

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 2 at r2 (raw file):

SUBSYSTEM=="misc",KERNEL=="enclave",MODE="0666"
SUBSYSTEM=="misc",KERNEL=="sgx_enclave",MODE="0666",SYMLINK+="sgx/enclave"

I strongly object to:

SYMLINK+=: We have no business in adding different files/symlinks to host's /dev. Instead we should make independently configurable both the path to the header and path to the device itself. Optimally the path to chardev should be #defined in the header itself, but this is outside of our control.
MODE=: This is not our decision if SGX should be available to all users or just some of them. I expect distributions to create a group (probably named sgx) and have the file 0660 and belonging to said group. Then local admin would add users to this group if they are to be permitted to use SGX on the host. But the group setup would have to happen in configuration/postinstall part of the distro package.
The sort number of 91-: Around 80 there are distro overrides, which is already too late, and especially 90+ is reserved for admin and local overrides. Packages usually have the files between 60 and 70, so I'd suggest 65.

As it stands, this is NAK. If you'd like to supply this file, please move it to contrib/udev-rules.d/65-graphene-sgx.rules (create the directories) and change mode to 0660. Yes, this will require more work to install and configure, but I don't want chmod 777 and equivalents in the repo.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 71 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, isn't it the SGX driver installation procedure that does this? Like, why would the user have to do this on a properly set up Linux system?

Either way, this instruction has no place in this file. It will not age well, because the config will eventually be supplied by distribution (either as standalone file, or likely just added to an existing file).

Pal/src/host/Linux-SGX/link-intel-driver.py, line 69 at r2 (raw file):

                if not os.path.exists(in_kernel_dev_node):
                    raise FileNotFoundError
            except FileNotFoundError:

Why this convoluted logic? Wouldn't if os.path.exists(...) and not os.path.exists(...): suffice?

johnvenn · 2021-01-19T02:33:03Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please let's also keep this version, to support the case where the user doesn't specify any SGX-driver directory (ISGX_DRIVER_PATH=""). And here the driver name must be changed to the new /dev/sgx_enclave.

udpated.

johnvenn · 2021-01-19T02:36:15Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 69 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

Why this convoluted logic? Wouldn't if os.path.exists(...) and not os.path.exists(...): suffice?

Both are ok. Just for indent purpose and make the line shorter.

dimakuv

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

a discussion (no related file):
@johnvenn Please use Reviewable for reviews and comments, it's much more convenient than normal GitHub functionality. (There is a button "Reviewable" in each PR description.)

a discussion (no related file):
Please address @woju's comments.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r1 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

udpated.

The sgx_in_kernel.h option is still missing...

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please keep this, to support the case where the user doesn't specify any SGX-driver directory (ISGX_DRIVER_PATH="").

Why did you change sgx_in_kernel.h to sgx.h? We only have sgx_in_kernel.h in the Graphene repo: https://github.com/oscarlab/graphene/blob/master/Pal/src/host/Linux-SGX/sgx_in_kernel.h

johnvenn · 2021-01-20T06:44:09Z

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@johnvenn Please use Reviewable for reviews and comments, it's much more convenient than normal GitHub functionality. (There is a button "Reviewable" in each PR description.)

yes, I have trouble in accessing this website several days ago, now I solved the issue.

johnvenn · 2021-01-20T06:44:30Z

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please address @woju's comments.

I will solve it today.

johnvenn · 2021-01-20T06:55:16Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 9 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

For Dcap driver from version 1.10+ -> For DCAP driver version 1.10+

Done.

johnvenn · 2021-01-20T06:55:40Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 8 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

There is a trailing space at the end of this line, please remove

Done.

johnvenn · 2021-01-20T06:55:50Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 18 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please keep this, to support older versions of DCAP

Done.

johnvenn · 2021-01-20T06:56:42Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why did you change sgx_in_kernel.h to sgx.h? We only have sgx_in_kernel.h in the Graphene repo: https://github.com/oscarlab/graphene/blob/master/Pal/src/host/Linux-SGX/sgx_in_kernel.h

yes, I am aware of this default header file, done to keep it

johnvenn · 2021-01-20T06:56:52Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r1 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

yes, I am aware of this default header file, done to keep it

Done.

johnvenn · 2021-01-20T06:57:05Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 69 at r2 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

Both are ok. Just for indent purpose and make the line shorter.

Done.

johnvenn · 2021-01-20T06:59:51Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 74 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove spaces

Done.

johnvenn · 2021-01-20T07:16:32Z

Pal/src/host/Linux-SGX/91-sgx-enclave.rules, line 2 at r2 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

I strongly object to:

SYMLINK+=: We have no business in adding different files/symlinks to host's /dev. Instead we should make independently configurable both the path to the header and path to the device itself. Optimally the path to chardev should be #defined in the header itself, but this is outside of our control.

MODE=: This is not our decision if SGX should be available to all users or just some of them. I expect distributions to create a group (probably named sgx) and have the file 0660 and belonging to said group. Then local admin would add users to this group if they are to be permitted to use SGX on the host. But the group setup would have to happen in configuration/postinstall part of the distro package.

The sort number of 91-: Around 80 there are distro overrides, which is already too late, and especially 90+ is reserved for admin and local overrides. Packages usually have the files between 60 and 70, so I'd suggest 65.

As it stands, this is NAK. If you'd like to supply this file, please move it to contrib/udev-rules.d/65-graphene-sgx.rules (create the directories) and change mode to 0660. Yes, this will require more work to install and configure, but I don't want chmod 777 and equivalents in the repo.

ok, working... in progress

johnvenn · 2021-01-20T07:21:24Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 10 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The sgx_in_kernel.h option is still missing...

added.

dimakuv

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

a discussion (no related file):
Please don't merge the master branch in your branch. We use the rebase git flow (and only when absolutely necessary to rebase). Please check https://graphene.readthedocs.io/en/latest/devel/contributing.html#pr-life-cycle.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 74 at r1 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

Done.

Not done.

Pal/src/host/Linux-SGX/link-intel-driver.py, line 11 at r3 (raw file):

        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',  # For Dcap driver 1.6+, but below 1.10
        'include/sgx_user.h':         '/dev/sgx/enclave',  # For Dcap driver 1.10+
        'include/uapi/asm/sgx.h':     '/dev/sgx/enclave',  # For upstreamed in-kernel SGX driver, kernel version 5.11+

Why did you revert back to /dev/sgx/enclave? I thought you wanted to add an option for /dev/sgx_enclave...

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r3 (raw file):

      - sgx_user.h for non-DCAP, older version of the driver
        (https://github.com/intel/linux-sgx-driver)
      - include/uapi/asm/sgx_oot.h for DCAP 1.6+ version of the driver

Please append but below 1.10

Pal/src/host/Linux-SGX/link-intel-driver.py, line 24 at r3 (raw file):

      - include/sgx_user.h for DCAP 1.10+ version of the driver
        (https://github.com/intel/SGXDataCenterAttestationPrimitives)
      - sgx.h for upstreamed SGX in-kernel driver from mainline kernel version 5.11

Please replace sgx.h -> include/uapi/asm/sgx.h

Pal/src/host/Linux-SGX/link-intel-driver.py, line 62 at r3 (raw file):

        sys.exit(1)

    if not isgx_driver_path: #using in-kernel SGX driver from kernel version 5.11

Please remove this added comment, we already explain that we use the in-kernel SGX driver in the comment below

johnvenn · 2021-01-20T10:44:28Z

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please don't merge the master branch in your branch. We use the rebase git flow (and only when absolutely necessary to rebase). Please check https://graphene.readthedocs.io/en/latest/devel/contributing.html#pr-life-cycle.

thanks a lot for the advice. Actually I was confusing because since we are now using this reviewable.io, I should not force push the fixes before all comments are resolved.

johnvenn · 2021-01-20T11:04:10Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 20 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please append but below 1.10

Done.

johnvenn · 2021-01-20T11:04:42Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 24 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please replace sgx.h -> include/uapi/asm/sgx.h

Done.

johnvenn · 2021-01-20T11:05:12Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 62 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this added comment, we already explain that we use the in-kernel SGX driver in the comment below

Done.

johnvenn · 2021-02-01T03:53:57Z

Documentation/sgx-intro.rst, line 98 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This whole block gets highlighted treating # as a comment marker, so the resulting HTML looks misleading (as these lines were comments). Please fix.
Also, I'm not sure which syntax highlighting scheme is selected by default, but it's probably Python, not sh, so you should use .. code-block:: sh.

I used "make html man" command to verify my original change, it's ok as I opened the file using firefox. Not sure what browser you are using to open this file.

johnvenn · 2021-02-01T04:18:38Z

Documentation/sgx-intro.rst, line 98 at r8 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

I used "make html man" command to verify my original change, it's ok as I opened the file using firefox. Not sure what browser you are using to open this file.

in my opinion, this is not code-block, it just lists the commands recommended for user to apply.

johnvenn · 2021-02-01T05:38:03Z

Documentation/sgx-intro.rst, line 95 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

I don't like the wording of this whole point, it's hard to read and repetitive. Something like this should be better:
- SGX support was upstreamed to the Linux mainline starting from 5.11.
  It currently supports only DCAP attestation. The driver is accessible through
  /dev/sgx_enclave and /dev/sgx_provision.
(plus leaving that "The following [...]" sentence as is, it's good in the current form)

Done.

johnvenn · 2021-02-01T05:38:14Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 77 at r8 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

please fix the double space

Done.

dimakuv

Reviewed 2 of 2 files at r9.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)

Documentation/sgx-intro.rst, line 98 at r8 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

in my opinion, this is not code-block, it just lists the commands recommended for user to apply.

Michal is right, this is a sh-formatted code block. Please add .. highlight:: sh line at the beginning of this whole file.

You can find an example of this in Documentation/quickstart.rst.

johnvenn · 2021-02-01T08:08:53Z

Documentation/sgx-intro.rst, line 98 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Michal is right, this is a sh-formatted code block. Please add .. highlight:: sh line at the beginning of this whole file.

You can find an example of this in Documentation/quickstart.rst.

thanks a lot, Dmitrii, I have updated. could you please review?

dimakuv

Reviewed 1 of 1 files at r10.
Reviewable status: all files reviewed, 3 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 @mkow)

Documentation/sgx-intro.rst, line 98 at r8 (raw file):

Previously, johnvenn (xiangping-INTEL) wrote…

thanks a lot, Dmitrii, I have updated. could you please review?

LGTM. Looks good to me!

dimakuv · 2021-02-01T08:14:08Z

Jenkins, test this please

dimakuv

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

Pal/src/host/Linux-SGX/link-intel-driver.py, line 12 at r10 (raw file):

        'include/sgx_user.h':         '/dev/sgx/enclave',  # For DCAP driver 1.10+
        'include/uapi/asm/sgx.h':     '/dev/sgx_enclave',  # For upstreamed in-kernel SGX driver, kernel version 5.11+
        'sgx_in_kernel.h':            '/dev/sgx/enclave',  # By default, using sgx_in_kernel.h in current dir of this script

Didn't notice before, but these lines now exceed the limit of 100 chars per line. Could you fix this by moving the comments above the lines? Like this:

         # For Non-DCAP, older versions of legacy OOT SGX driver
        'sgx_user.h':                 '/dev/isgx',
         # For DCAP driver 1.6+, but below 1.10
        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',
         # For DCAP driver 1.10+
        'include/sgx_user.h':         '/dev/sgx/enclave',
         # For upstreamed in-kernel SGX driver, kernel version 5.11+
        'include/uapi/asm/sgx.h':     '/dev/sgx_enclave',
         # By default, using sgx_in_kernel.h in current dir of this script
        'sgx_in_kernel.h':            '/dev/sgx/enclave',

mkow

Reviewed 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: all 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: ITL) (waiting on @johnvenn)

johnvenn · 2021-02-01T14:36:12Z

Pal/src/host/Linux-SGX/link-intel-driver.py, line 12 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Didn't notice before, but these lines now exceed the limit of 100 chars per line. Could you fix this by moving the comments above the lines? Like this:

         # For Non-DCAP, older versions of legacy OOT SGX driver
        'sgx_user.h':                 '/dev/isgx',
         # For DCAP driver 1.6+, but below 1.10
        'include/uapi/asm/sgx_oot.h': '/dev/sgx/enclave',
         # For DCAP driver 1.10+
        'include/sgx_user.h':         '/dev/sgx/enclave',
         # For upstreamed in-kernel SGX driver, kernel version 5.11+
        'include/uapi/asm/sgx.h':     '/dev/sgx_enclave',
         # By default, using sgx_in_kernel.h in current dir of this script
        'sgx_in_kernel.h':            '/dev/sgx/enclave',

Done.

mkow

Reviewed 1 of 1 files at r11.
Reviewable status: all 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: ITL) (waiting on @dimakuv)

Intel SGX driver was upstreamed in Linux version 5.11. There, the SGX device is exposed as `/dev/sgx_enclave` instead of `/dev/sgx/enclave`. This commit updates link-intel-driver.py to recognize this new name.

dimakuv

Reviewed 1 of 1 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv

Reviewable status: complete! all files reviewed, all discussions resolved

a discussion (no related file):
@johnvenn Thanks for the PR! It was approved, and we will rebase and merge it now. Please do not do anything with this PR and your branch from now on.

dimakuv

Reviewed 1 of 1 files at r12.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

dimakuv · 2021-02-01T19:20:40Z

Jenkins, test this please

mkow

Reviewed 1 of 1 files at r12.
Reviewable status: complete! all files reviewed, all discussions resolved

dimakuv suggested changes Jan 14, 2021

View reviewed changes

dimakuv reviewed Jan 15, 2021

View reviewed changes

mkow requested a review from woju January 15, 2021 11:44

woju suggested changes Jan 15, 2021

View reviewed changes

dimakuv suggested changes Jan 19, 2021

View reviewed changes

johnvenn force-pushed the issue_2007 branch 2 times, most recently from 51ba347 to f4c9cdf Compare January 20, 2021 06:40

dimakuv suggested changes Jan 20, 2021

View reviewed changes

johnvenn dismissed stale reviews from woju and dimakuv via 82b77d8 February 1, 2021 04:21

johnvenn force-pushed the issue_2007 branch from 26c994a to 82b77d8 Compare February 1, 2021 04:21

dimakuv suggested changes Feb 1, 2021

View reviewed changes

johnvenn force-pushed the issue_2007 branch from d0db2ad to 1fa48fa Compare February 1, 2021 08:06

dimakuv approved these changes Feb 1, 2021

View reviewed changes

dimakuv suggested changes Feb 1, 2021

View reviewed changes

mkow previously approved these changes Feb 1, 2021

View reviewed changes

johnvenn dismissed mkow’s stale review via af25969 February 1, 2021 14:24

johnvenn force-pushed the issue_2007 branch 3 times, most recently from a8028e0 to 016ceb7 Compare February 1, 2021 14:33

mkow previously approved these changes Feb 1, 2021

View reviewed changes

[Pal/Linux-SGX] Recognize upstreamed Intel SGX driver

c59a143

Intel SGX driver was upstreamed in Linux version 5.11. There, the SGX device is exposed as `/dev/sgx_enclave` instead of `/dev/sgx/enclave`. This commit updates link-intel-driver.py to recognize this new name.

dimakuv approved these changes Feb 1, 2021

View reviewed changes

dimakuv previously approved these changes Feb 1, 2021

View reviewed changes

dimakuv dismissed stale reviews from mkow and themself via c59a143 February 1, 2021 19:19

dimakuv force-pushed the issue_2007 branch from 016ceb7 to c59a143 Compare February 1, 2021 19:19

dimakuv approved these changes Feb 1, 2021

View reviewed changes

mkow approved these changes Feb 1, 2021

View reviewed changes

dimakuv merged commit c59a143 into gramineproject:master Feb 1, 2021

sync with upstreamed SGX in-kernel driver from mainline 5.11 #2084

sync with upstreamed SGX in-kernel driver from mainline 5.11 #2084

Conversation

johnvenn commented Jan 14, 2021 • edited Loading

Description of the changes

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

johnvenn commented Jan 15, 2021 • edited Loading

johnvenn commented Jan 15, 2021

johnvenn commented Jan 15, 2021

isgx_driver_path = os.path.dirname(os.path.abspath(file))

debin-yang commented Jan 15, 2021

debin-yang commented Jan 15, 2021 • edited Loading

woju left a comment

Choose a reason for hiding this comment

johnvenn commented Jan 19, 2021

johnvenn commented Jan 19, 2021

dimakuv left a comment

Choose a reason for hiding this comment

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

dimakuv left a comment

Choose a reason for hiding this comment

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Jan 20, 2021

johnvenn commented Feb 1, 2021

johnvenn commented Feb 1, 2021

johnvenn commented Feb 1, 2021

johnvenn commented Feb 1, 2021

dimakuv left a comment

Choose a reason for hiding this comment

johnvenn commented Feb 1, 2021

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv commented Feb 1, 2021

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

johnvenn commented Feb 1, 2021

mkow 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

dimakuv left a comment

Choose a reason for hiding this comment

dimakuv commented Feb 1, 2021

mkow left a comment

Choose a reason for hiding this comment

johnvenn commented Jan 14, 2021 •

edited

Loading

johnvenn commented Jan 15, 2021 •

edited

Loading

debin-yang commented Jan 15, 2021 •

edited

Loading