Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Corrected check for in-kernel driver #2427

Closed
wants to merge 1 commit into from

Conversation

ScottR-Intel
Copy link
Contributor

@ScottR-Intel ScottR-Intel commented Jun 7, 2021

Description of the changes

The in-kernel driver by itself doesn't create the /dev/sgx folder any longer. This is done by the PSW installers now. So, if the PSW isn't yet installed when running the tool, it will fail the in-kernel driver check. Modifying the tool to check for /dev/sgx_provision /dev/sgx_enclave which is what the in-kernel driver creates.


This change is Reviewable

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

bool sgx_driver_loaded() {
    // /dev/isgx is for LKM version, /dev/sgx_enclave and /dev/sgx_provision are for in-kernel support.
    return file_exists("/dev/isgx") || (file_exists("/dev/sgx_enclave") && file_exists("/dev/sgx_provision"));

This will return false on DCAP < 1.10. Do we care about that?


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

bool sgx_driver_loaded() {
    // /dev/isgx is for LKM version, /dev/sgx_enclave and /dev/sgx_provision are for in-kernel support.
    return file_exists("/dev/isgx") || (file_exists("/dev/sgx_enclave") && file_exists("/dev/sgx_provision"));

We don't need to check for sgx_provision.

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

This will return false on DCAP < 1.10. Do we care about that?

+1 to woju's request. Why not keep the older /dev/sgx as a third option here?


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

We don't need to check for sgx_provision.

It doesn't hurt to check for it, it's a part of the SGX driver installation. I would keep it.

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@woju woju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 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 @woju)


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

It doesn't hurt to check for it, it's a part of the SGX driver installation. I would keep it.

IDK, we don't use it, right? But no strong opinion, unblocking.

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Pal/src/host/Linux-SGX/tools/is-sgx-available/is_sgx_available.cpp, line 169 at r1 (raw file):

Previously, woju (Wojtek Porczyk) wrote…

IDK, we don't use it, right? But no strong opinion, unblocking.

Graphene doesn't use it. But the tool in question (is_sgx_available) is unrelated to Graphene -- it just so happened that it is built and shipped with Graphene (because one of the ITL people wrote this handy tool, and we just have it now).

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @ScottR-Intel and @woju)

a discussion (no related file):
Whoops, I forgot about this PR and made my own fix for this issue (#2438). Sorry @ScottR-Intel, I didn't intend to take over this PR :)
Anyways, I prefer my version, because it doesn't remove the checks for /dev/sgx, which AFAIK still has users. But if you want to introduce a similar change here, then we can keep your PR and I'll close mine. If you want to do so, please also reformat the comment like I did in #2438.


@mkow mkow changed the title Corrected check for in-kernel driver. Corrected check for in-kernel driver Jun 13, 2021
Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @ScottR-Intel and @woju)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Whoops, I forgot about this PR and made my own fix for this issue (#2438). Sorry @ScottR-Intel, I didn't intend to take over this PR :)
Anyways, I prefer my version, because it doesn't remove the checks for /dev/sgx, which AFAIK still has users. But if you want to introduce a similar change here, then we can keep your PR and I'll close mine. If you want to do so, please also reformat the comment like I did in #2438.

I suggest to go with Michal's #2438.


Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants