Re: [PATCH v2 0/3] x86/sgx: eextend ioctl
On 2021-04-14 12:52, Jarkko Sakkinen wrote: > On Mon, Apr 12, 2021 at 10:59:56AM +0200, Raoul Strackx wrote: >> Creation of an SGX enclave consists of three steps. First, a new enclave >> environment is created by the ECREATE leaf function. Some enclave settings >> are specified at this step by passing an SGX Enclave Control Structure >> (SECS) that contains the enclave MRENCLAVE, MRSIGNER, etc. This >> instruction also starts a cryptographic log of the enclave being built. >> (This log should eventually result in the MRENCLAVE.) Second, pages are >> added to the enclave. The EADD leaf function copies 4KB data to an empty >> EPC page. The cryptographic log records (among other properties) the >> location and access rights of the page being added. It _does not_ include >> an entry of the page content. When the enclave writer wishes to ensure the >> content of (a part of) the enclave page as well, she must use the EEXTEND >> leaf function. Extending the enclave cryptographic log can only be done >> per 256 bytes. Extending the log with a full 4K page thus requires 16 >> invocations of the EEXTEND leaf function. It is however up to the enclave >> developer to decide if and how enclave memory is added to the >> cryptographic log. EEXTEND functions may be issued only for relevant parts >> of an enclave page, may happen only after all pages have been added, and >> so on. Finally, the enclave is finalized by the EINIT leaf function. Any >> new invocations of the EADD or EEXTEND leaf functions will result in a >> fault. With EINIT a number of checks are performed as well. The >> cryptographic hash of the final cryptographic log is compared to the >> MRENCLAVE field of the SECS structure passed to the ECREATE leaf function >> (see step one). The signature (MRSIGNER) over this MRENCLAVE is verified >> as well. When all checks pass, the enclave loading is complete and it >> enters the executable state. > > Who do you expect to read this paragraph, seriously? What do you mean? There was a request for more architectural details in the cover letter. > >> The SGX driver currently only supports extending the cryptographic log as >> part of the EADD leaf function and _must_ cover complete 4K pages. >> Enclaves not constructed within these constraints, currently cannot be >> loaded on the Linux platform. Trying to do so will result in a different >> cryptographic log; the MRENCLAVE specified at enclave creation time will >> not match the cryptographic log kept by the processor and EINIT will fail. >> This poses practical problems: >> - The current driver does not fully support all possible SGXv1 enclaves. >> It creates a separation between enclaves that run everywhere and >> enclaves that run everywhere, except on Linux. This includes enclaves >> already in use on other systems today. >> - It limits optimizations loaders are able to perform. For example, by >> only measuring relevant parts of enclave pages, load time can be >> minimized. >> >> This patch set adds a new ioctl to enable userspace to execute EEXTEND >> leaf functions per 256 bytes of enclave memory. With this patch in place, >> Linux will be able to build all valid SGXv1 enclaves. >> >> See additional discussion at: >> https://lore.kernel.org/linux-sgx/20200220221038.ga26...@linux.intel.com/ >> T/#m93597f53d354201e72e26d93a968f167fcdf5930 >> >> >> Raoul Strackx (3): >> x86/sgx: Adding eextend ioctl >> x86/sgx: Fix compatibility issue with OPENSSL < 1.1.0 >> x86/sgx: eextend ioctl selftest >> >> arch/x86/include/uapi/asm/sgx.h | 11 + >> arch/x86/kernel/cpu/sgx/ioctl.c | 81 >> ++++- >> tools/testing/selftests/sgx/defines.h | 1 + >> tools/testing/selftests/sgx/load.c | 57 +++ >> tools/testing/selftests/sgx/main.h | 1 + >> tools/testing/selftests/sgx/sigstruct.c | 43 - >> 6 files changed, 154 insertions(+), 40 deletions(-) >> >> -- >> 2.7.4 >> >> > > /Jarkko > -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v2 0/3] x86/sgx: eextend ioctl
On 2021-04-12 18:47, Dave Hansen wrote: > On 4/12/21 9:41 AM, Jethro Beekman wrote: >> Yes this still doesn't let one execute all possible ECREATE, EADD, EEXTEND, >> EINIT sequences. > > OK, so we're going in circles now. > > I don't believe we necessarily *WANT* or need Linux to support "all > possible ECREATE, EADD, EEXTEND, EINIT sequences". Yet, it's what is > being used to justify this series without any other justification. > > It's going to be a different story if you bring me a real enclave that > *REALLY* wants to do this for good reasons. > It's still not clear to me what your motivations are for trying to keep Linux incompatible with the rest of the world. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v2 0/3] x86/sgx: eextend ioctl
On 2021-04-12 18:40, Dave Hansen wrote:
> On 4/12/21 8:58 AM, Jethro Beekman wrote:
>> On 2021-04-12 17:36, Dave Hansen wrote:
>>> On 4/12/21 1:59 AM, Raoul Strackx wrote:
>>>> This patch set adds a new ioctl to enable userspace to execute EEXTEND
>>>> leaf functions per 256 bytes of enclave memory. With this patch in place,
>>>> Linux will be able to build all valid SGXv1 enclaves.
>>> This didn't cover why we need a *NEW* ABI for this instead of relaxing
>>> the page alignment rules in the existing one.
>>>
>> In executing the ECREATE, EADD, EEXTEND, EINIT sequence, you currently have
>> 2 options for EADD/EEXTEND using the SGX_IOC_ENCLAVE_ADD_PAGES ioctl:
>> - execute EADD on any address
>> - execute EADD on any address followed by 16× EEXTEND for that address span
>
> I think you forgot a key piece of the explanation here. The choice as
> to whether you just EADD or EADD+16xEEXTEND is governed by the addition
> of the: SGX_PAGE_MEASURE flag.
>
>> Could you be more specific on how you're suggesting that the current ioctl
>> is modified to in addition support the following?
>> - execute EEXTEND on any address
>
> I'm still not convinced you *NEED* EEXTEND on arbitrary addresses.
>
> Right now, we have (roughly):
>
>ioctl(ADD_PAGES, ptr, PAGE_SIZE, MEASURE)
>
> which translates in the kernel to:
>
> __eadd(ptr, epc)
> if (flags & MEASURE) {
> for (i = 0; i < PAGE_SIZE/256; i++)
> __eextend(epc + i*256);
> }
>
> Instead, we could allow add_arg.src and add_arg.offset to be
> non-page-aligned. Then, we still do the same __eadd(), but modify the
> __eextend() loop to only cover the actual range referred to by 'add_arg'.
>
> The downside is that you only get a single range of measured data per
> page. Let's say a 'X' means measured (EEXTEND'ed) and '_' means not.
> You could have patterns like:
>
>
> or
> XXX_
> or
> ____
>
> but not:
>
> _X_X_X_X_X_X_X_X
> or
> _XX_
>
>
> Is that a problem?
>
Yes this still doesn't let one execute all possible ECREATE, EADD, EEXTEND,
EINIT sequences.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v2 0/3] x86/sgx: eextend ioctl
On 2021-04-12 17:36, Dave Hansen wrote: > On 4/12/21 1:59 AM, Raoul Strackx wrote: >> This patch set adds a new ioctl to enable userspace to execute EEXTEND >> leaf functions per 256 bytes of enclave memory. With this patch in place, >> Linux will be able to build all valid SGXv1 enclaves. > > This didn't cover why we need a *NEW* ABI for this instead of relaxing > the page alignment rules in the existing one. > In executing the ECREATE, EADD, EEXTEND, EINIT sequence, you currently have 2 options for EADD/EEXTEND using the SGX_IOC_ENCLAVE_ADD_PAGES ioctl: - execute EADD on any address - execute EADD on any address followed by 16× EEXTEND for that address span Could you be more specific on how you're suggesting that the current ioctl is modified to in addition support the following? - execute EEXTEND on any address -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-02 22:48, Dave Hansen wrote: > On 4/2/21 1:20 PM, Jethro Beekman wrote: >> On 2021-04-02 21:50, Dave Hansen wrote: >>> Again, how does this save space? >>> >>> Are you literally talking about the temporary cost of allocating *one* page? >> >> No I'm talking about the amount of disk space/network traffic needed >> to distribute the application. > > Am I just horribly confused about how executable formats work? > > Executables go through an SGX loader that copies them into SGX memory > with the kernel's help. > > That executable can have *ANY* format, really. > > Then, a loader needs to read that format and turn it into data that can > be shoved into the kernel. Sure, you can define any compression format or conversion rules between executable formats. But the native “executable format” for SGX is very clearly defined in the Intel SDM as a specific sequence of ECREATE, EADD, EEXTEND and EINIT calls. It's that sequence that's used for loading the enclave and it's that sequence that's used for code signing. So when I say save space I mean save space in the native format. Not EEXTENDing unnecessarily also reduces enclave load time if you're looking for additional arguments. > The simplest way to do this is to just > mmap() the executable and point the kernel ioctl()'s at the executable's > pages one-by-one. > > The other way a loader *could* work is to copy the data to a temporary > location and then hand the temporary location to the SGX ioctl()s. > > Let's say the kernel *REQUIRED* page-aligned and page-sized ioctl() > arguments forever. If an executable had a 256-byte-sized chunk of data, > all the loader would have to do is allocate a page, copy the data in > there, and then pass that whole page into the ioctl(). > > In other words, the loading restrictions (page alignment) have little to > nothing to do with the format of the thing loading the executable. > > But, in no way does a kernel page-size-based ABI *REQUIRE* that an > underlying binary format represent things only in page-sized chunks. > Look at how many page-sized executables there are in /bin. Yet, they > can only be mapped into the address space in PAGE_SIZE increments. > >>>>> Does any actual, real-world enclave want this functionality? Why? >>> >>> I didn't see an answer on this one. >> >> Yes, we have enclaves that use this functionality. They already exist >> so they can't be changed (without changing the measurement) and we'd >> like to stop using the out of tree driver as soon as possible. >> However, we are not able to load the enclaves. > OK, so please give this series another shot. Please explain why you > *ACTUALLY* need it and what the goals are. Please explain why you can't > just relax the restrictions of the existing add ioctl() to take > > As far as I can tell, there are only two coherent arguments for this > functionality: > 1. It makes the loader simpler so that it doesn't need temporary pages > 2. It would allow old enclaves created with non-upstream-Linux SGX >implementations to end up with the same signatures on these >implementations as upstream Linux. > > I find both of those pretty weak arguments. Doing #2 just for the > out-of-tree Linux implementation also encourages folks to establish ABI > out of the tree and then foist it on upstream later. That's not super cool. > But, I guess this would be nice to the folks that have gone to the > trouble of building SGX enclaves for all these years with no upstream > support. Linux doesn't exist in a vacuum. People who write SGX applications write those applications for SGX, not for Linux. For Linux to claim to support SGX version 1, it should support all SGX version 1 applications. The current implementation is not SGX version 1, it's some limited subset of it. The cost to Linux for supporting all of SGX version 1 seems excessively low. SGX defines lots of things and you may not see the use case for all of this immediately. No one has a usecase for creating enclaves with SECS.SSAFRAMESIZE = 1000 or TCS.NSSA = 3. Why did you not demand checks for this in the ECREATE/EADD ioctls? > > I'll try to look at it with fresh eyes once this is in place. > -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-04 18:04, Jarkko Sakkinen wrote: > On Fri, Apr 02, 2021 at 08:31:19PM +0200, Jethro Beekman wrote: >> On 2021-04-02 17:53, Dave Hansen wrote: >>> On 4/2/21 1:38 AM, Jethro Beekman wrote: >>>>> So, we're talking here about pages that have been EEADDED, but for >>>>> which we do not want to include the entire contents of the page? >>>>> Do these contents always include the beginning of the page, or can >>>>> the holes be anywhere? >>>> Holes can be anywhere, and EEXTEND calls need not be sequential in >>>> memory address or even relate to the most recently EADDed page. >>> >>> I think you're referring to the SGX architecture itself here. The >>> architecture permits this, right? >> >> Yes. >> >>> But, why would an enclave loader application ever do this? >> >> e.g. to save space >> >>> Is this something we want to support in Linux? >> >> Why not? Is there a good reason to not fully support this part of the CPU >> architecture? > > Yes, in generic sense :-) > > If one would disagree, that would be same as saying that everything should > execute in ring-0 because that only gives "full support". How is that the same? Please make an effort to reasonably interpret what I'm saying. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-02 21:50, Dave Hansen wrote: > > On 4/2/21 12:38 PM, Jethro Beekman wrote: >> On 2021-04-02 20:42, Dave Hansen wrote: >>> On 4/2/21 11:31 AM, Jethro Beekman wrote: >>>> On 2021-04-02 17:53, Dave Hansen wrote: >>>>> But, why would an enclave loader application ever do this? >>>> >>>> e.g. to save space >>> >>> How does this save space, exactly? What space does it save? >> >> With the current driver interface, if you want to communicate an >> application binary that has pages that are at least partially >> measured, you need to communicate the entire page (to ensure the same >> measurement for the entire page), even though most of that page's contents >> are irrelevant to the application. > > Again, how does this save space? > > Are you literally talking about the temporary cost of allocating *one* page? No I'm talking about the amount of disk space/network traffic needed to distribute the application. > >>> We don't blindly support CPU features in Linux. They need to do >>> something *useful*. I'm still missing what this does which is >>> useful. >> >> Enclaves can only be loaded exactly as specified by the developer, > this is the whole point of the measurement architecture. By not > supporting arbitrary EADD/EEXTEND flows, the SGX application ecosystem > is effectively split into two: SGX applications that run everywhere and > SGX applications that run everywhere except on Linux. So, the "something > useful" is being compatible. Linux has plenty of features that exist > solely for compatibility with other systems, such as binfmt_misc. > > That's a mildly compelling argument. Is it theoretical or practical? > Are folks *practically* going to run the same enclave binaries on Linux > and Windows? This is certainly practical. As you mention below, enclaves don't interact with the OS, so this should really be the default. It's quite puzzling to me that the Intel SGX SDK /doesn't/ let you do this (easily, it's possible with https://github.com/fortanix/rust-sgx/blob/master/sgxs-tools/src/bin/isgx-pe2sgx.rs). The x86_64-fortanix-unknown-sgx target that's part of Rust is fully designed to be OS-agnostic. > > I guess the enclave never interacts with the OS directly, so this is > _possible_. But, are enclaves really that divorced from the "runtimes" > which *are* OS-specific? > >>> Does any actual, real-world enclave want this functionality? Why? > > I didn't see an answer on this one. Yes, we have enclaves that use this functionality. They already exist so they can't be changed (without changing the measurement) and we'd like to stop using the out of tree driver as soon as possible. However, we are not able to load the enclaves. > >>> P.S. There are plenty of things you can do with the SGX >>> architecture that we probably won't ever implement in Linux. >> >> How so? > > For example, the architecture allows swapping VA pages and guest enclave > pages. But, we may never do either of those. > This is not an application-visible part of the architecture.* Resource management is squarely in the kernel's purview. (* I suppose you could argue that without VA paging the practical enclave size is limited to 512× the EPC size, so ~46GiB for systems with 128MiB PRM Considering the overhead of EPC paging in general, that really seems more of a theoretical limitations) -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-02 20:42, Dave Hansen wrote: > On 4/2/21 11:31 AM, Jethro Beekman wrote: >> On 2021-04-02 17:53, Dave Hansen wrote: >>> On 4/2/21 1:38 AM, Jethro Beekman wrote: >>>>> So, we're talking here about pages that have been EEADDED, but for >>>>> which we do not want to include the entire contents of the page? >>>>> Do these contents always include the beginning of the page, or can >>>>> the holes be anywhere? >>>> Holes can be anywhere, and EEXTEND calls need not be sequential in >>>> memory address or even relate to the most recently EADDed page. >>> >>> I think you're referring to the SGX architecture itself here. The >>> architecture permits this, right? >> >> Yes. >> >>> But, why would an enclave loader application ever do this? >> >> e.g. to save space > > How does this save space, exactly? What space does it save? With the current driver interface, if you want to communicate an application binary that has pages that are at least partially measured, you need to communicate the entire page (to ensure the same measurement for the entire page), even though most of that page's contents are irrelevant to the application. > > Let's say I want to add 4352 bytes of data to an enclave. Today, I have > to page-align the beginning and end of that 4352 bytes, and call the add > ioctl() to add the two resulting pages. It consumes two EPC pages. > > With EEXTEND, if I want to add the data, I only need to page-align the > beginning of the data. I call add_page on the first page, then eextend > on the 256 bytes. It consumes two EPC pages. > > I guess you can argue that this saves padding out the second page, which > would *theoretically* temporarily eat up one extra page of non-enclave > memory and the cost of a 256-byte memcpy. > >>> Is this something we want to support in Linux? >> >> Why not? Is there a good reason to not fully support this part of the >> CPU architecture? > > We don't blindly support CPU features in Linux. They need to do > something *useful*. I'm still missing what this does which is useful. Enclaves can only be loaded exactly as specified by the developer, this is the whole point of the measurement architecture. By not supporting arbitrary EADD/EEXTEND flows, the SGX application ecosystem is effectively split into two: SGX applications that run everywhere and SGX applications that run everywhere except on Linux. So, the "something useful" is being compatible. Linux has plenty of features that exist solely for compatibility with other systems, such as binfmt_misc. > > Does any actual, real-world enclave want this functionality? Why? > > P.S. There are plenty of things you can do with the SGX architecture > that we probably won't ever implement in Linux. > How so? Linux doesn't normally put arbitrary restrictions on what userspace does when it's not interacting with the kernel. Userspace is free to load its own memory contents and execute all the instructions it wants. AFAIK, besides this EEXTEND issue there is nothing of the SGX architecture that SGX applications may be using that's not supported. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-02 17:53, Dave Hansen wrote: > On 4/2/21 1:38 AM, Jethro Beekman wrote: >>> So, we're talking here about pages that have been EEADDED, but for >>> which we do not want to include the entire contents of the page? >>> Do these contents always include the beginning of the page, or can >>> the holes be anywhere? >> Holes can be anywhere, and EEXTEND calls need not be sequential in >> memory address or even relate to the most recently EADDed page. > > I think you're referring to the SGX architecture itself here. The > architecture permits this, right? Yes. > But, why would an enclave loader application ever do this? e.g. to save space > Is this something we want to support in Linux? Why not? Is there a good reason to not fully support this part of the CPU architecture? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-01 20:40, Dave Hansen wrote: > On 4/1/21 10:49 AM, Raoul Strackx wrote: >> On 4/1/21 6:11 PM, Dave Hansen wrote: >>> On 4/1/21 7:56 AM, Raoul Strackx wrote: >>>> SOLUTION OF THIS PATCH >>>> This patch adds a new ioctl to enable userspace to execute EEXTEND leaf >>>> functions per 256 bytes of enclave memory. This enables enclaves to be >>>> build as specified by enclave providers. >>> I think tying the user ABI to the SGX architecture this closely is a >>> mistake. >>> >>> Do we need another ioctl() or can we just relax the existing add_pages >>> ioctl() to allow unaligned addresses? >>> >> I've considered this. In order to do an EEXTEND without an EADD, we'd >> need to add a flag DONT_ADD_PAGES flag to `add_pages` ioctl as well. Two >> separate ioctls, one for adding, another for extending made more sense >> to me. > > So, we're talking here about pages that have been EEADDED, but for which > we do not want to include the entire contents of the page? Do these > contents always include the beginning of the page, or can the holes be > anywhere? Holes can be anywhere, and EEXTEND calls need not be sequential in memory address or even relate to the most recently EADDed page. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH RESEND 0/3] x86/sgx: eextend ioctl
On 2021-04-01 18:11, Dave Hansen wrote: > On 4/1/21 7:56 AM, Raoul Strackx wrote: >> >> SOLUTION OF THIS PATCH >> This patch adds a new ioctl to enable userspace to execute EEXTEND leaf >> functions per 256 bytes of enclave memory. This enables enclaves to be >> build as specified by enclave providers. > > I think tying the user ABI to the SGX architecture this closely is a > mistake. > > Do we need another ioctl() or can we just relax the existing add_pages > ioctl() to allow unaligned addresses? > Some version of the driver used to pass a 16-bit bitset of which of the 16 256-byte parts of a 4096-byte page to measure. This was removed at some point in favor of a separate ioctl to be added later. That is this. See the discussion linked in the cover letter. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v2] ACPI: scan: Fix a Hyper-V Linux VM panic caused by buffer overflow
On 2021-01-08 08:23, Dexuan Cui wrote:
> Linux VM on Hyper-V crashes with the latest mainline:
>
> [4.069624] detected buffer overflow in strcpy
> [4.077733] kernel BUG at lib/string.c:1149!
> ..
> [4.085819] RIP: 0010:fortify_panic+0xf/0x11
> ...
> [4.085819] Call Trace:
> [4.085819] acpi_device_add.cold.15+0xf2/0xfb
> [4.085819] acpi_add_single_object+0x2a6/0x690
> [4.085819] acpi_bus_check_add+0xc6/0x280
> [4.085819] acpi_ns_walk_namespace+0xda/0x1aa
> [4.085819] acpi_walk_namespace+0x9a/0xc2
> [4.085819] acpi_bus_scan+0x78/0x90
> [4.085819] acpi_scan_init+0xfa/0x248
> [4.085819] acpi_init+0x2c1/0x321
> [4.085819] do_one_initcall+0x44/0x1d0
> [4.085819] kernel_init_freeable+0x1ab/0x1f4
>
> This is because of the recent buffer overflow detection in the
> commit 6a39e62abbaf ("lib: string.h: detect intra-object overflow in
> fortified string functions")
>
> Here acpi_device_bus_id->bus_id can only hold 14 characters, while the
> the acpi_device_hid(device) returns a 22-char string
> "HYPER_V_GEN_COUNTER_V1".
>
> Per ACPI Spec v6.2, Section 6.1.5 _HID (Hardware ID), if the ID is a
> string, it must be of the form AAA or , i.e. 7 chars or 8
> chars.
>
> The field bus_id in struct acpi_device_bus_id was originally defined as
> char bus_id[9], and later was enlarged to char bus_id[15] in 2007 in the
> commit bb0958544f3c ("ACPI: use more understandable bus_id for ACPI devices")
>
> Fix the issue by changing the field bus_id to const char *, and use
> kstrdup_const() to initialize it.
>
> Signed-off-by: Dexuan Cui
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=210449
Tested-By: Jethro Beekman
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Fwd: [Bug 210449] New: acpi_device_add: buffer overflow in strcpy
This regression (boot failure) is now in 5.11.0-rc2 -- Jethro Beekman | Fortanix Forwarded Message Subject: [Bug 210449] New: acpi_device_add: buffer overflow in strcpy Date: Wed, 02 Dec 2020 02:16:38 -0800 From: bugzilla-dae...@bugzilla.kernel.org https://bugzilla.kernel.org/show_bug.cgi?id=210449 Bug ID: 210449 Summary: acpi_device_add: buffer overflow in strcpy Product: ACPI Version: 2.5 Kernel Version: next-20201201 Hardware: All OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Config-Other Assignee: acpi_config-ot...@kernel-bugs.osdl.org Reporter: ker...@jbeekman.nl Regression: No [0.00] Linux version 5.10.0-rc6-next-20201201 (root@sgx-kernel-build) (gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609, GNU ld (GNU Binutils for Ubuntu) 2.26.1) #7 SMP Wed Dec 2 09:18:07 UTC 2020 [0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc6-next-20201201 root=UUID=4ec1dc08-af51-4080-b927-561417afaa77 ro console=tty1 console=ttyS0 earlyprintk=ttyS0 [0.00] KERNEL supported cpus: [0.00] Intel GenuineIntel [0.00] AMD AuthenticAMD [0.00] Hygon HygonGenuine [0.00] Centaur CentaurHauls [0.00] zhaoxin Shanghai [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers' [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' [0.00] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers' [0.00] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR' [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 [0.00] x86/fpu: xstate_offset[3]: 832, xstate_sizes[3]: 64 [0.00] x86/fpu: xstate_offset[4]: 896, xstate_sizes[4]: 64 [0.00] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using 'compacted' format. [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: [mem 0x-0x0009] usable [0.00] BIOS-e820: [mem 0x000c-0x000f] reserved [0.00] BIOS-e820: [mem 0x0010-0x3ed4bfff] usable [0.00] BIOS-e820: [mem 0x3ed4c000-0x3ed4cfff] ACPI data [0.00] BIOS-e820: [mem 0x3ed4d000-0x3ee7afff] usable [0.00] BIOS-e820: [mem 0x3ee7b000-0x3ee99fff] ACPI data [0.00] BIOS-e820: [mem 0x3ee9a000-0x3eef1fff] usable [0.00] BIOS-e820: [mem 0x3eef2000-0x3ef1afff] reserved [0.00] BIOS-e820: [mem 0x3ef1b000-0x3ff9afff] usable [0.00] BIOS-e820: [mem 0x3ff9b000-0x3fff2fff] reserved [0.00] BIOS-e820: [mem 0x3fff3000-0x3fffafff] ACPI data [0.00] BIOS-e820: [mem 0x3fffb000-0x3fffefff] ACPI NVS [0.00] BIOS-e820: [mem 0x3000-0x3fff] usable [0.00] BIOS-e820: [mem 0x0001-0x0001bfff] usable [0.00] BIOS-e820: [mem 0x0001c020-0x0001c1bf] reserved [0.00] BIOS-e820: [mem 0x0001c1e0-0x0001c1ff] reserved [0.00] printk: bootconsole [earlyser0] enabled [0.00] NX (Execute Disable) protection: active [0.00] efi: EFI v2.70 by Microsoft [0.00] efi: ACPI=0x3fffa000 ACPI 2.0=0x3fffa014 SMBIOS=0x3ffd8000 SMBIOS 3.0=0x3ffd6000 MEMATTR=0x3f9e0218 RNG=0x3ffda818 [0.00] efi: seeding entropy pool [0.00] SMBIOS 3.1.0 present. [0.00] DMI: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 06/17/2020 [0.00] Hypervisor detected: Microsoft Hyper-V [0.00] Hyper-V: features 0x2e7f, hints 0x60c2c, misc 0xed7b2 [0.00] Hyper-V Host Build:18362-10.0-3-0.3186 [0.00] Hyper-V: LAPIC Timer Frequency: 0xc3500 [0.00] tsc: Marking TSC unstable due to running on Hyper-V [0.00] Hyper-V: Using hypercall for remote TLB flush [0.00] clocksource: hyperv_clocksource_tsc_page: mask: 0x max_cycles: 0x24e6a1710, max_idle_ns: 440795202120 ns [0.02] tsc: Detected 3696.012 MHz processor [0.002124] last_pfn = 0x1c max_arch_pfn = 0x4 [0.004543] x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT Memory KASLR using RDRAND RDTSC... [0.008675] last_pfn = 0x4 max_arch_pfn = 0x4 [0.021667] check: Scanning 1 areas for low memory corruption [0.024119] Using GB pages for direct mapping [0.026644] Secure boot disabled [0.029161] RAMDISK: [mem 0x2c75c000-0x2f097fff] [0.033563] ACPI: Early table checksum verification disabled [0.037148] ACPI: RSDP 0x3FFFA014 24 (v02 VRTUAL
Re: [PATCH v40 00/24] Intel SGX foundations
On 2020-11-04 15:54, Jarkko Sakkinen wrote: *snip* > Jarkko Sakkinen (14): > x86/sgx: Add SGX architectural data structures > x86/sgx: Add wrappers for ENCLS functions > x86/cpu/intel: Add nosgx kernel parameter > x86/sgx: Add SGX page allocator functions > x86/sgx: Add SGX misc driver interface > x86/sgx: Add SGX_IOC_ENCLAVE_CREATE > x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES > x86/sgx: Add SGX_IOC_ENCLAVE_INIT > x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION > selftests/x86: Add a selftest for SGX > x86/sgx: Add a page reclaimer > x86/sgx: Add ptrace() support for the SGX driver > docs: x86/sgx: Document SGX kernel architecture > x86/sgx: Update MAINTAINERS > > Sean Christopherson (10): > x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections > x86/cpufeatures: x86/msr: Add Intel SGX hardware bits > x86/cpufeatures: x86/msr: Add Intel SGX Launch Control hardware bits > x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX > x86/cpu/intel: Detect SGX support > mm: Add 'mprotect' hook to struct vm_operations_struct > x86/vdso: Add support for exception fixup in vDSO functions > x86/fault: Add helper function to sanitize error code > x86/traps: Attempt to fixup exceptions in vDSO before signaling > x86/vdso: Implement a vDSO for Intel SGX enclave call I tested Jarkko's public git master branch at the time of writing (patch number, commit): 01 3dbc955 Acked-By: Jethro Beekman 02 0fb18ca Acked-By: Jethro Beekman 03 8f7ab60 Acked-By: Jethro Beekman 04 358d170 Acked-By: Jethro Beekman 05 0c64b4c Acked-By: Jethro Beekman 06 b0bacb5 Acked-By: Jethro Beekman 07 e131efe Acked-By: Jethro Beekman 08 5984a2c Acked-By: Jethro Beekman 09 93b27a8 Acked-By: Jethro Beekman 10 8ec6c36 Acked-By: Jethro Beekman 11 1e67355 Tested-By: Jethro Beekman 12 9f48d02 Tested-By: Jethro Beekman 13 53f7984 Tested-By: Jethro Beekman 14 5ab939b Tested-By: Jethro Beekman 15 6caa47ae Acked-By: Jethro Beekman 16 3106551 Acked-By: Jethro Beekman 17 7193709 Acked-By: Jethro Beekman 18 9c7d634 Acked-By: Jethro Beekman 19 cad6a3d Tested-By: Jethro Beekman 20 0dadc6b Acked-By: Jethro Beekman 21 e396b6f Acked-By: Jethro Beekman 22 bfcbc47 Tested-By: Jethro Beekman 23 7a0da40 Acked-By: Jethro Beekman 24 a644dc1 Acked-By: Jethro Beekman -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v40 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-11-04 15:54, Jarkko Sakkinen wrote: > From: Sean Christopherson > > Enclaves encounter exceptions for lots of reasons: everything from enclave > page faults to NULL pointer dereferences, to system calls that must be > “proxied” to the kernel from outside the enclave. > > In addition to the code contained inside an enclave, there is also > supporting code outside the enclave called an “SGX runtime”, which is > virtually always implemented inside a shared library. The runtime helps > build the enclave and handles things like *re*building the enclave if it > got destroyed by something like a suspend/resume cycle. > > The rebuilding has traditionally been handled in SIGSEGV handlers, > registered by the library. But, being process-wide, shared state, signal > handling and shared libraries do not mix well. > > Introduce a vDSO function call that wraps the enclave entry functions > (EENTER/ERESUME functions of the ENCLU instruciton) and returns information > about any exceptions to the caller in the SGX runtime. > > Instead of generating a signal, the kernel places exception information in > RDI, RSI and RDX. The kernel-provided userspace portion of the vDSO handler > will place this information in a user-provided buffer or trigger a > user-provided callback at the time of the exception. > > The vDSO function calling convention uses the standard RDI RSI, RDX, RCX, > R8 and R9 registers. This makes it possible to declare the vDSO as a C > prototype, but other than that there is no specific support for SystemV > ABI. Things like storing XSAVE are the responsibility of the enclave and > the runtime. I suppose this may be covered under "no specific support for SystemV ABI" but with sgx_enclave_run.user_handler=NULL, R12~R15 *will* get clobbered when __vdso_sgx_enter_enclave returns from an SGX AEX. IMO this makes the whole "try to be like System V ABI" rather useless, but I suppose it doesn't matter too much. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v40 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-11-04 15:54, Jarkko Sakkinen wrote:
> From: Sean Christopherson
>
> Enclaves encounter exceptions for lots of reasons: everything from enclave
> page faults to NULL pointer dereferences, to system calls that must be
> “proxied” to the kernel from outside the enclave.
>
> In addition to the code contained inside an enclave, there is also
> supporting code outside the enclave called an “SGX runtime”, which is
> virtually always implemented inside a shared library. The runtime helps
> build the enclave and handles things like *re*building the enclave if it
> got destroyed by something like a suspend/resume cycle.
>
> The rebuilding has traditionally been handled in SIGSEGV handlers,
> registered by the library. But, being process-wide, shared state, signal
> handling and shared libraries do not mix well.
>
> Introduce a vDSO function call that wraps the enclave entry functions
> (EENTER/ERESUME functions of the ENCLU instruciton) and returns information
> about any exceptions to the caller in the SGX runtime.
>
> Instead of generating a signal, the kernel places exception information in
> RDI, RSI and RDX. The kernel-provided userspace portion of the vDSO handler
> will place this information in a user-provided buffer or trigger a
> user-provided callback at the time of the exception.
>
> The vDSO function calling convention uses the standard RDI RSI, RDX, RCX,
> R8 and R9 registers. This makes it possible to declare the vDSO as a C
> prototype, but other than that there is no specific support for SystemV
> ABI. Things like storing XSAVE are the responsibility of the enclave and
> the runtime.
>
> Suggested-by: Andy Lutomirski
> Acked-by: Jethro Beekman
> Tested-by: Jethro Beekman
> Signed-off-by: Sean Christopherson
> Co-developed-by: Cedric Xing
> Signed-off-by: Cedric Xing
> Co-developed-by: Jarkko Sakkinen
> Signed-off-by: Jarkko Sakkinen
> ---
> Changes from v39:
> * Relayout out the user handler documentation: return values are described
> in sgx_enclave_user_handler_t keneldoc and broad description is given
> in struct sgx_enclave_run kerneldoc.
> * Rename @leaf as @function, given that we want to speak consistently
> about ENCLS and ENCLU functions.
> * Reorder user_handler and user_data as the last fields in
> sgx_enclave_run, as they are an extension to the basic functionality.
>
> arch/x86/entry/vdso/Makefile| 2 +
> arch/x86/entry/vdso/vdso.lds.S | 1 +
> arch/x86/entry/vdso/vsgx.S | 151
> arch/x86/include/asm/enclu.h| 9 ++
> arch/x86/include/uapi/asm/sgx.h | 91 +++
> 5 files changed, 254 insertions(+)
> create mode 100644 arch/x86/entry/vdso/vsgx.S
> create mode 100644 arch/x86/include/asm/enclu.h
>
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index 2ad757fb3c23..9915fbd34264 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -27,6 +27,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y
> vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
> vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
> vobjs32-y += vdso32/vclock_gettime.o
> +vobjs-$(VDSO64-y)+= vsgx.o
>
> # files to link into kernel
> obj-y+= vma.o extable.o
> @@ -98,6 +99,7 @@ $(vobjs): KBUILD_CFLAGS := $(filter-out
> $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS
> CFLAGS_REMOVE_vclock_gettime.o = -pg
> CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
> CFLAGS_REMOVE_vgetcpu.o = -pg
> +CFLAGS_REMOVE_vsgx.o = -pg
>
> #
> # X32 processes use x32 vDSO to access 64bit kernel data.
> diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
> index 36b644e16272..4bf48462fca7 100644
> --- a/arch/x86/entry/vdso/vdso.lds.S
> +++ b/arch/x86/entry/vdso/vdso.lds.S
> @@ -27,6 +27,7 @@ VERSION {
> __vdso_time;
> clock_getres;
> __vdso_clock_getres;
> + __vdso_sgx_enter_enclave;
> local: *;
> };
> }
> diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S
> new file mode 100644
> index ..86a0e94f68df
> --- /dev/null
> +++ b/arch/x86/entry/vdso/vsgx.S
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "extable.h"
> +
> +/* Relative to %rbp. */
> +#define SGX_ENCLAVE_OFFSET_OF_RUN16
> +
> +/* The offsets relative to struct sgx_enclave_run. */
> +#define SGX_ENCLAVE_RUN_TCS 0
> +#define SGX_ENCLAVE_RUN_LEAF 8
> +#define SGX_ENCLAVE_RUN_EXCEP
Re: [PATCH v40 19/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-11-04 15:54, Jarkko Sakkinen wrote:
> From: Sean Christopherson
>
> Enclaves encounter exceptions for lots of reasons: everything from enclave
> page faults to NULL pointer dereferences, to system calls that must be
> “proxied” to the kernel from outside the enclave.
>
> In addition to the code contained inside an enclave, there is also
> supporting code outside the enclave called an “SGX runtime”, which is
> virtually always implemented inside a shared library. The runtime helps
> build the enclave and handles things like *re*building the enclave if it
> got destroyed by something like a suspend/resume cycle.
>
> The rebuilding has traditionally been handled in SIGSEGV handlers,
> registered by the library. But, being process-wide, shared state, signal
> handling and shared libraries do not mix well.
>
> Introduce a vDSO function call that wraps the enclave entry functions
> (EENTER/ERESUME functions of the ENCLU instruciton) and returns information
> about any exceptions to the caller in the SGX runtime.
>
> Instead of generating a signal, the kernel places exception information in
> RDI, RSI and RDX. The kernel-provided userspace portion of the vDSO handler
> will place this information in a user-provided buffer or trigger a
> user-provided callback at the time of the exception.
>
> The vDSO function calling convention uses the standard RDI RSI, RDX, RCX,
> R8 and R9 registers. This makes it possible to declare the vDSO as a C
> prototype, but other than that there is no specific support for SystemV
> ABI. Things like storing XSAVE are the responsibility of the enclave and
> the runtime.
>
> Suggested-by: Andy Lutomirski
> Acked-by: Jethro Beekman
> Tested-by: Jethro Beekman
> Signed-off-by: Sean Christopherson
> Co-developed-by: Cedric Xing
> Signed-off-by: Cedric Xing
> Co-developed-by: Jarkko Sakkinen
> Signed-off-by: Jarkko Sakkinen
> ---
> Changes from v39:
> * Relayout out the user handler documentation: return values are described
> in sgx_enclave_user_handler_t keneldoc and broad description is given
> in struct sgx_enclave_run kerneldoc.
> * Rename @leaf as @function, given that we want to speak consistently
> about ENCLS and ENCLU functions.
> * Reorder user_handler and user_data as the last fields in
> sgx_enclave_run, as they are an extension to the basic functionality.
>
> arch/x86/entry/vdso/Makefile| 2 +
> arch/x86/entry/vdso/vdso.lds.S | 1 +
> arch/x86/entry/vdso/vsgx.S | 151
> arch/x86/include/asm/enclu.h| 9 ++
> arch/x86/include/uapi/asm/sgx.h | 91 +++
> 5 files changed, 254 insertions(+)
> create mode 100644 arch/x86/entry/vdso/vsgx.S
> create mode 100644 arch/x86/include/asm/enclu.h
>
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index 2ad757fb3c23..9915fbd34264 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -27,6 +27,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y
> vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
> vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
> vobjs32-y += vdso32/vclock_gettime.o
> +vobjs-$(VDSO64-y)+= vsgx.o
>
> # files to link into kernel
> obj-y+= vma.o extable.o
> @@ -98,6 +99,7 @@ $(vobjs): KBUILD_CFLAGS := $(filter-out
> $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS
> CFLAGS_REMOVE_vclock_gettime.o = -pg
> CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
> CFLAGS_REMOVE_vgetcpu.o = -pg
> +CFLAGS_REMOVE_vsgx.o = -pg
>
> #
> # X32 processes use x32 vDSO to access 64bit kernel data.
> diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
> index 36b644e16272..4bf48462fca7 100644
> --- a/arch/x86/entry/vdso/vdso.lds.S
> +++ b/arch/x86/entry/vdso/vdso.lds.S
> @@ -27,6 +27,7 @@ VERSION {
> __vdso_time;
> clock_getres;
> __vdso_clock_getres;
> + __vdso_sgx_enter_enclave;
> local: *;
> };
> }
> diff --git a/arch/x86/entry/vdso/vsgx.S b/arch/x86/entry/vdso/vsgx.S
> new file mode 100644
> index ..86a0e94f68df
> --- /dev/null
> +++ b/arch/x86/entry/vdso/vsgx.S
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "extable.h"
> +
> +/* Relative to %rbp. */
> +#define SGX_ENCLAVE_OFFSET_OF_RUN16
> +
> +/* The offsets relative to struct sgx_enclave_run. */
> +#define SGX_ENCLAVE_RUN_TCS 0
> +#define SGX_ENCLAVE_RUN_LEAF 8
> +#define SGX_ENCLAVE_RUN_EXCEP
Re: [PATCH v39 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION
On 2020-10-23 12:17, Jarkko Sakkinen wrote: > On Tue, Oct 20, 2020 at 02:19:26PM -0700, Dave Hansen wrote: >> On 10/2/20 9:50 PM, Jarkko Sakkinen wrote: >>> + * Failure to explicitly request access to a restricted attribute will >>> cause >>> + * sgx_ioc_enclave_init() to fail. Currently, the only restricted >>> attribute >>> + * is access to the PROVISION_KEY. >> >> Could we also justify why access is restricted, please? Maybe: >> >> Access is restricted because PROVISION_KEY is burned uniquely >> into each each processor, making it a perfect unique identifier >> with privacy and fingerprinting implications. >> >> Are there any other reasons for doing it this way? > > AFAIK, if I interperet the SDM correctl, PROVISION_KEY and > PROVISION_SEALING_KEY also have random salt added, i.e. they change > every boot cycle. > > There is "RAND = yes" on those keys in Table 40-64 of Intel SDM volume > 3D :-) > This is nonsense. The whole point of sealing keys is that they don't change every boot. If did they they'd have no value over enclave memory. RAND means that the KEYID field from the KEYREQUEST is included in the derivation (as noted in the source row of the table you looked at). -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-10-06 04:57, Sean Christopherson wrote:
> On Sat, Oct 03, 2020 at 07:50:56AM +0300, Jarkko Sakkinen wrote:
>> From: Sean Christopherson
>> +/* Validate that the reserved area contains only zeros. */
>> +push%rax
>> +push%rbx
>> +mov $SGX_ENCLAVE_RUN_RESERVED_START, %rbx
>> +1:
>> +mov (%rcx, %rbx), %rax
>> +cmpq$0, %rax
>> +jne .Linvalid_input
>> +
>> +add $8, %rbx
>> +cmpq$SGX_ENCLAVE_RUN_RESERVED_END, %rbx
>> +jne 1b
>> +pop %rbx
>> +pop %rax
>
> This can more succinctly be (untested):
>
> movqSGX_ENCLAVE_RUN_RESERVED_1(%rbp), %rbx
> orq SGX_ENCLAVE_RUN_RESERVED_2(%rbp), %rbx
> orq SGX_ENCLAVE_RUN_RESERVED_3(%rbp), %rbx
> jnz .Linvalid_input
>
> Note, %rbx is getting clobbered anyways, no need to save/restore it.
>
>> diff --git a/arch/x86/include/uapi/asm/sgx.h
>> b/arch/x86/include/uapi/asm/sgx.h
>> index b6ba036a9b82..3dd2df44d569 100644
>> --- a/arch/x86/include/uapi/asm/sgx.h
>> +++ b/arch/x86/include/uapi/asm/sgx.h
>> @@ -74,4 +74,102 @@ struct sgx_enclave_provision {
>> __u64 attribute_fd;
>> };
>>
>> +struct sgx_enclave_run;
>> +
>> +/**
>> + * typedef sgx_enclave_user_handler_t - Exit handler function accepted by
>> + * __vdso_sgx_enter_enclave()
>> + * @run:Pointer to the caller provided struct sgx_enclave_run
>> + *
>> + * The register parameters contain the snapshot of their values at enclave
>> + * exit
>> + *
>> + * Return:
>> + * 0 or negative to exit vDSO
>> + * positive to re-enter enclave (must be EENTER or ERESUME leaf)
>> + */
>> +typedef int (*sgx_enclave_user_handler_t)(long rdi, long rsi, long rdx,
>> + long rsp, long r8, long r9,
>> + struct sgx_enclave_run *run);
>> +
>> +/**
>> + * struct sgx_enclave_run - the execution context of
>> __vdso_sgx_enter_enclave()
>> + * @tcs:TCS used to enter the enclave
>> + * @user_handler: User provided callback run on exception
>> + * @user_data: Data passed to the user handler
>> + * @leaf: The ENCLU leaf we were at (EENTER/ERESUME/EEXIT)
>> + * @exception_vector: The interrupt vector of the exception
>> + * @exception_error_code: The exception error code pulled out of the stack
>> + * @exception_addr: The address that triggered the exception
>> + * @reservedReserved for possible future use
>> + */
>> +struct sgx_enclave_run {
>> +__u64 tcs;
>> +__u64 user_handler;
>> +__u64 user_data;
>> +__u32 leaf;
>
> I am still very strongly opposed to omitting exit_reason. It is not at all
> difficult to imagine scenarios where 'leaf' alone is insufficient for the
> caller or its handler to deduce why the CPU exited the enclave. E.g. see
> Jethro's request for intercepting interrupts.
Not entirely sure what this has to do with my request, I just expect to see
leaf=ERESUME in this case, I think? E.g. as you would see in EAX when calling
ENCLU.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: Can we credibly make vdso_sgx_enter_enclave() pleasant to use?
On 2020-09-25 18:55, Andy Lutomirski wrote:
> vdso_sgx_enter_enclave() sucks. I don't think anyone seriously likes
> it, but maybe it's the best we can do.
I don't agree that it sucks. I think it's pretty good. My goal is to 1) support
existing enclaves and 2) have it as close to ENCLU as possible. Goal 2 since I
want to use it in cross-platform code and other platforms don't have this kind
of call, so I'm forced to use assembly anyway.
The only concerns I have are that I need 256 bytes of memory (but I can put
that on the stack I suppose) and the automatic restarting, but I've agreed to
postpone the latter issue post-merge.
Now I'm speaking from a perspective of someone who's not planning to use the
callback/user stack, so perhaps people using those features might think the
current implementation is not that great.
>
> I'm wondering if it's worth trying to do better. Here's what I'd like
> if I could wave a magic wand:
>
> struct sgx_enclave_run {
>__u64 tcs;
>__u32 flags;
>__u32 exit_reason;
>
> /*
> * These values are exposed to the enclave on entry, and the values
> * left behind by the enclave are returned here.
> * Some enclaves might write to memory pointed to by rsp.
> */
>__u64 rsp, rbp, r8, r9, r10, r11, r12, r13, r14, r15;
>/* Maybe other regs too? */
I think it's fine to add a mechanism to pass rsp & rbp, and I support
innovation in this space. But we should aim to keep the other register passing
as close as possible to the instruction/what the current implementation does.
For the other state, it certainly encompasses more than just the 16 GPRs.
There's also rflags, XMM, etc. Future processors might add even more state.
Capturing all state in this structure seems unnecessary.
On 2020-09-25 21:09, Sean Christopherson wrote:
> For the code, given the constraints of SGX and the number of runtimes we're
> enabling, I don't think it's bad at all. It's not hard to maintain, there are
> no horrendous hacks, and it doesn't play games with the caller's state, i.e.
> there's no additional magic required. In other words, I really like that we
> have in hand _works_, and works for a variety of runtimes and their ABIs.
>
> The API isn't glorious, but it's not awful either.
Yup.
On 2020-09-25 22:20, Andy Lutomirski wrote:
> On the other hand, if we had some confidence that the existing corpus
> of enclaves plays games with RSP but not RBP
Pretty sure this is the case.
> languages that aren't quite as C-like as C. In a language with
> stackless coroutines or async/await or continuations or goroutines,
> this could all get quite awkward. Sure, a really nice Rust or Go SGX
> untrusted runtime could just declare that it won't support enclaves
> that touch the stack, but that's a bit of an unfortunate restriction
> given that removing stack access from an existing enclave will
> inevitably change MRENCLAVE.
I can say with confidence that v38 proposal can be used by async Rust code if
the enclave doesn't use the user stack.
> If everyone wants to tell me that what we have now (plus .cfi
> annotations and perhaps a CET fix) is good enough, then so be it.
Good enough for me.
On 2020-09-26 00:29, Sean Christopherson wrote:
> We do have this confidence, because it's required by the current version
> of the VDSO.
Do you mean “it's required by the current version of the VDSO” and nobody has
complained about it?
> But the callback is optional...
Yup.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v38 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-09-25 13:17, Jarkko Sakkinen wrote: > On Fri, Sep 25, 2020 at 10:39:58AM +0200, Jethro Beekman wrote: >> On 2020-09-25 03:00, Jarkko Sakkinen wrote: >>> End result: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-sgx.git/tree/arch/x86/include/uapi/asm/sgx.h >>> >>> I'm wondering this sentence: >>> >>> "The calling convention is custom and does not follow System V x86-64 ABI." >>> >>> AFAIK, now the vDSO is fully C-callable but waiting for feedback before >>> removing it. >> >> It's C-callable *iff your enclave follows the System V x86-64 ABI*. In >> addition, all registers not clobbered by the SGX ISA are passed to the >> enclave, not just those specified as parameter passing registers in >> the ABI. This is intentional to make the vDSO function usable in >> applications that use the current flexibility of ENCLU. > > Hold on, I really want to fix this bit of documentation before sending > any new version, so I'll explain how I understand it. > > I think it is just SystemV ABI call with six parameters in the usual > GPRs (rdi, rsi, rdx, rcx, r8, r9). > > I'm looking at vdso_sgx_enter_enclave. > > What I'm not getting? This can't be observed from looking at the code in vdso_sgx_enter_enclave, because what makes this "custom" is the absence of code or code in the enclave. If you call vdso_sgx_enter_enclave() from C but your enclave doesn't respect the ABI (for example, it clobbers callee-saved registers), your code will break. But if you call vdso_sgx_enter_enclave from assembly, you can use enclaves with any ABI as long as your assembly and the enclave agree on that ABI. Alternatively, when using assembly, I can pass stuff to the enclave in registers besides rdi, rsi, rdx, rcx, r8, r9, just as if I were manually calling ENCLU from assembly. The vDSO assembly has been carefully written to support both scenarios by only using rax, rbx, rcx, rbp, rsp and passing rdi, rsi, rdx, r8, r9 (and everything else). > + * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the > + * x86-64 ABI, e.g. doesn't handle XSAVE state. Except for non-volatile > + * general purpose registers, EFLAGS.DF, and RSP alignment, > preserving/setting > + * state in accordance with the x86-64 ABI is the responsibility of the > enclave > + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C > + * code without careful consideration by both the enclave and its runtime. > + * > + * All general purpose registers except RAX, RBX and RCX are passed as-is to > + * the enclave. RAX, RBX and RCX are consumed by EENTER and ERESUME and are > + * loaded with @leaf, asynchronous exit pointer, and @tcs respectively. Perhaps this should be updated to read "All general purpose registers except RAX, RBX, RCX, RBP and RSP ..." -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v38 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-09-25 03:00, Jarkko Sakkinen wrote: > End result: > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-sgx.git/tree/arch/x86/include/uapi/asm/sgx.h > > I'm wondering this sentence: > > "The calling convention is custom and does not follow System V x86-64 ABI." > > AFAIK, now the vDSO is fully C-callable but waiting for feedback before > removing it. It's C-callable *iff your enclave follows the System V x86-64 ABI*. In addition, all registers not clobbered by the SGX ISA are passed to the enclave, not just those specified as parameter passing registers in the ABI. This is intentional to make the vDSO function usable in applications that use the current flexibility of ENCLU. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v38 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT
On 2020-09-22 10:29, Borislav Petkov wrote: > On Mon, Sep 21, 2020 at 12:17:00PM -0700, Sean Christopherson wrote: >> That was effectively my original suggestion as well, check for a stale cache >> and retry indefinitely. I capitulated because it did feel like I was being >> overly paranoid. I'm obviously ok going the retry indefinitely route :-). >> >> https://lkml.kernel.org/r/20180904163546.ga5...@linux.intel.com > > Right, so if EINIT is so expensive, why does it matter how many cyccles > WRMSR has? I.e., you don't really need to cache - you simply write the 4 > MSRs and you're done. Simple. > > As to "indefinitely" - caller can increment a counter which counts > how many times it returned SGX_INVALID_EINITTOKEN. I guess when it > reaches some too high number which should not be reached during normal > usage patterns, you can give up and issue a message to say that counter > reached max retries or so but other than that, you should be ok. That > thing is running interruptible in a loop anyway... I don't see why you'd need to retry indefinitely. Yes the MSRs may not match the cached value for “reasons”, but if after you've written them once it still doesn't work, clearly either 1) an “unhelpful” VMM is actively messing with the MSRs which I'd say is at best a VMM bug or 2) there was an EPC reset and your enclave is now invalid anyway, so no need to EINIT. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-08-19 15:33, Nathaniel McCallum wrote: > On Tue, Aug 18, 2020 at 12:44 PM Jarkko Sakkinen > wrote: >> >> On Tue, Aug 18, 2020 at 11:15:32AM -0400, Nathaniel McCallum wrote: >>> That seems like overkill to me. I'm just asking for one additional mov >>> instruction. :) >> >> I started to consider eBPF since the complexity and constraints of the >> callback look like an overkill and without doubt will be a burden to >> maintain. > > That feels to me like more complexity just to move the existing > complexity from one place to another. > Agreed that BPF doesn't make things better. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-08-11 00:23, Sean Christopherson wrote: > Another thought would be to wrap sgx_enclave_exception in a struct to give > room for supporting additional exit information (if such a thing ever pops > up) and to allow the caller to opt in to select behavior, e.g. Jethro's > request to invoke the exit handler on IRQ exits. This is basically the > equivalent of "struct kvm_run", minus the vCPU/enclave state. Actually, the flag I need is “return from the vdso on IRQ exits” (See Andy's email about preferring returns over callbacks). But maybe the flag should be “interpret IRQ exit as a normal exit” and let it be handled the same as any other exit based on whether an exit handler fnptr was passed or not. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v35 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-07-14 11:56, Jarkko Sakkinen wrote:
> On Tue, Jul 14, 2020 at 09:30:03AM +0200, Jethro Beekman wrote:
>> On 2020-07-07 05:37, Jarkko Sakkinen wrote:
>>> From: Sean Christopherson
>>>
>>> An SGX runtime must be aware of the exceptions, which happen inside an
>>> enclave. Introduce a vDSO call that wraps EENTER/ERESUME cycle and returns
>>> the CPU exception back to the caller exactly when it happens.
>>>
>>> Kernel fixups the exception information to RDI, RSI and RDX. The SGX call
>>> vDSO handler fills this information to the user provided buffer or
>>> alternatively trigger user provided callback at the time of the exception.
>>>
>>> The calling convention is custom and does not follow System V x86-64 ABI.
>>>
>>> Suggested-by: Andy Lutomirski
>>> Acked-by: Jethro Beekman
>>> Tested-by: Jethro Beekman
>>> Signed-off-by: Sean Christopherson
>>> Co-developed-by: Cedric Xing
>>> Signed-off-by: Cedric Xing
>>> Signed-off-by: Jarkko Sakkinen
>>> ---
>>> arch/x86/entry/vdso/Makefile | 2 +
>>> arch/x86/entry/vdso/vdso.lds.S | 1 +
>>> arch/x86/entry/vdso/vsgx_enter_enclave.S | 131 +++
>>> arch/x86/include/asm/enclu.h | 8 ++
>>> arch/x86/include/uapi/asm/sgx.h | 98 +
>>> 5 files changed, 240 insertions(+)
>>> create mode 100644 arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> create mode 100644 arch/x86/include/asm/enclu.h
>>>
>>> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
>>> index ebe82b7aecda..f71ad5ebd0c4 100644
>>> --- a/arch/x86/entry/vdso/Makefile
>>> +++ b/arch/x86/entry/vdso/Makefile
>>> @@ -29,6 +29,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y
>>> vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
>>> vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
>>> vobjs32-y += vdso32/vclock_gettime.o
>>> +vobjs-$(VDSO64-y) += vsgx_enter_enclave.o
>>>
>>> # files to link into kernel
>>> obj-y += vma.o extable.o
>>> @@ -100,6 +101,7 @@ $(vobjs): KBUILD_CFLAGS := $(filter-out
>>> $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS
>>> CFLAGS_REMOVE_vclock_gettime.o = -pg
>>> CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
>>> CFLAGS_REMOVE_vgetcpu.o = -pg
>>> +CFLAGS_REMOVE_vsgx_enter_enclave.o = -pg
>>>
>>> #
>>> # X32 processes use x32 vDSO to access 64bit kernel data.
>>> diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
>>> index 36b644e16272..4bf48462fca7 100644
>>> --- a/arch/x86/entry/vdso/vdso.lds.S
>>> +++ b/arch/x86/entry/vdso/vdso.lds.S
>>> @@ -27,6 +27,7 @@ VERSION {
>>> __vdso_time;
>>> clock_getres;
>>> __vdso_clock_getres;
>>> + __vdso_sgx_enter_enclave;
>>> local: *;
>>> };
>>> }
>>> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> b/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> new file mode 100644
>>> index ..be7e467e1efb
>>> --- /dev/null
>>> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
>>> @@ -0,0 +1,131 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include "extable.h"
>>> +
>>> +#define EX_LEAF0*8
>>> +#define EX_TRAPNR 0*8+4
>>> +#define EX_ERROR_CODE 0*8+6
>>> +#define EX_ADDRESS 1*8
>>> +
>>> +.code64
>>> +.section .text, "ax"
>>> +
>>> +SYM_FUNC_START(__vdso_sgx_enter_enclave)
>>> + /* Prolog */
>>> + .cfi_startproc
>>> + push%rbp
>>> + .cfi_adjust_cfa_offset 8
>>> + .cfi_rel_offset %rbp, 0
>>> + mov %rsp, %rbp
>>> + .cfi_def_cfa_register %rbp
>>> + push%rbx
>>> + .cfi_rel_offset %rbx, -8
>>> +
>>> + mov %ecx, %eax
>>> +.Lenter_enclave:
>>> + /* EENTER <= leaf <= ERESUME */
>>> + cmp $EENTER, %eax
>>> + jb .Linvalid_leaf
>>> + cmp $ERESUME, %eax
>>> + ja .Linvalid_leaf
>>> +
>>> + /* Load TCS and AEP */
>>> + mov 0x10(%rbp), %rbx
>&
Re: [PATCH v35 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On 2020-07-07 05:37, Jarkko Sakkinen wrote:
> From: Sean Christopherson
>
> An SGX runtime must be aware of the exceptions, which happen inside an
> enclave. Introduce a vDSO call that wraps EENTER/ERESUME cycle and returns
> the CPU exception back to the caller exactly when it happens.
>
> Kernel fixups the exception information to RDI, RSI and RDX. The SGX call
> vDSO handler fills this information to the user provided buffer or
> alternatively trigger user provided callback at the time of the exception.
>
> The calling convention is custom and does not follow System V x86-64 ABI.
>
> Suggested-by: Andy Lutomirski
> Acked-by: Jethro Beekman
> Tested-by: Jethro Beekman
> Signed-off-by: Sean Christopherson
> Co-developed-by: Cedric Xing
> Signed-off-by: Cedric Xing
> Signed-off-by: Jarkko Sakkinen
> ---
> arch/x86/entry/vdso/Makefile | 2 +
> arch/x86/entry/vdso/vdso.lds.S | 1 +
> arch/x86/entry/vdso/vsgx_enter_enclave.S | 131 +++
> arch/x86/include/asm/enclu.h | 8 ++
> arch/x86/include/uapi/asm/sgx.h | 98 +
> 5 files changed, 240 insertions(+)
> create mode 100644 arch/x86/entry/vdso/vsgx_enter_enclave.S
> create mode 100644 arch/x86/include/asm/enclu.h
>
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index ebe82b7aecda..f71ad5ebd0c4 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -29,6 +29,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y
> vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
> vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
> vobjs32-y += vdso32/vclock_gettime.o
> +vobjs-$(VDSO64-y)+= vsgx_enter_enclave.o
>
> # files to link into kernel
> obj-y+= vma.o extable.o
> @@ -100,6 +101,7 @@ $(vobjs): KBUILD_CFLAGS := $(filter-out
> $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS
> CFLAGS_REMOVE_vclock_gettime.o = -pg
> CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
> CFLAGS_REMOVE_vgetcpu.o = -pg
> +CFLAGS_REMOVE_vsgx_enter_enclave.o = -pg
>
> #
> # X32 processes use x32 vDSO to access 64bit kernel data.
> diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
> index 36b644e16272..4bf48462fca7 100644
> --- a/arch/x86/entry/vdso/vdso.lds.S
> +++ b/arch/x86/entry/vdso/vdso.lds.S
> @@ -27,6 +27,7 @@ VERSION {
> __vdso_time;
> clock_getres;
> __vdso_clock_getres;
> + __vdso_sgx_enter_enclave;
> local: *;
> };
> }
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> new file mode 100644
> index ..be7e467e1efb
> --- /dev/null
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "extable.h"
> +
> +#define EX_LEAF 0*8
> +#define EX_TRAPNR0*8+4
> +#define EX_ERROR_CODE0*8+6
> +#define EX_ADDRESS 1*8
> +
> +.code64
> +.section .text, "ax"
> +
> +SYM_FUNC_START(__vdso_sgx_enter_enclave)
> + /* Prolog */
> + .cfi_startproc
> + push%rbp
> + .cfi_adjust_cfa_offset 8
> + .cfi_rel_offset %rbp, 0
> + mov %rsp, %rbp
> + .cfi_def_cfa_register %rbp
> + push%rbx
> + .cfi_rel_offset %rbx, -8
> +
> + mov %ecx, %eax
> +.Lenter_enclave:
> + /* EENTER <= leaf <= ERESUME */
> + cmp $EENTER, %eax
> + jb .Linvalid_leaf
> + cmp $ERESUME, %eax
> + ja .Linvalid_leaf
> +
> + /* Load TCS and AEP */
> + mov 0x10(%rbp), %rbx
> + lea .Lasync_exit_pointer(%rip), %rcx
> +
> + /* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> +.Lasync_exit_pointer:
> +.Lenclu_eenter_eresume:
> + enclu
After thinking about this some more, I'd like to come back to this setup. Prior
discussion at https://lkml.org/lkml/2018/11/2/597 . I hope I'm not derailing
the discussion so much as to delay the patch set :(
I previously mentioned “Userspace may want fine-grained control over enclave
scheduling” as a reason userspace may want to specify a different AEP, but gave
a bad example. Here's a better example: If I'm running my enclave in an M:N
threading model (where M user threads run N TCSs, with N > M), an AEX is a good
oppurtunity to switch contexts. Yes, I could implement this with alarm() or so,
but that adds overhead while missing out on a lot of opportunities for context
switching.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v32 00/21] Intel SGX foundations
Looks like we missed the boat for 5.8. Can we get this ready for 5.9? -- Jethro Beekman | Fortanix On 2020-06-01 09:51, Jarkko Sakkinen wrote: > Intel(R) SGX is a set of CPU instructions that can be used by applications > to set aside private regions of code and data. The code outside the enclave > is disallowed to access the memory inside the enclave by the CPU access > control. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx > > v32: > * v31 contained not fully cleaned up main.c after merger of main.c and > reclaim.c. Fixed in this version. > * Rebased to v5.7. Forgot to do this for v31. > > v31: > * Unset SGX_ENCL_IOCTL in the error path of checking encl->flags in order > to prevent leaving it set and thus block any further ioctl calls. > * Added missing cleanup_srcu_struct() call to sgx_encl_release(). > * Take encl->lock in sgx_encl_add_page() in order to prevent races with > the page reclaimer. > * Fix a use-after-free bug from the page reclaimer. Call kref_put() for > encl->refcount only after putting enclave page back to the active page > list because it could be the last ref to the enclave. > * Filter any CPU disallowed values from sigstruct->vendor > SGX_IOC_ENCLAVE_INIT. > * Use bits 0-7 of page descriptor for the EPC section index. This > should be enough for long term needs. > * Refined naming for functions that allocate and free EPC pages to > be more sound and consistent. > * Merge main.c and reclaim.c into one. > > v30: > Bunch of tags added. No actual code changes. > > v29: > * The selftest has been moved to selftests/sgx. Because SGX is an execution > environment of its own, it really isn't a great fit with more "standard" > x86 tests. > > The RSA key is now generated on fly and the whole signing process has > been made as part of the enclave loader instead of signing the enclave > during the compilation time. > > Finally, the enclave loader loads now the test enclave directly from its > ELF file, which means that ELF file does not need to be coverted as raw > binary during the build process. > * Version the mm_list instead of using on synchronize_mm() when adding new > entries. We hold the write lock for the mm_struct, and dup_mm() can thus > deadlock with the page reclaimer, which could hold the lock for the old > mm_struct. > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can > be used to reserve the address range. Now /dev/sgx supports only opaque > mappings to the (initialized) enclave data. > * Make the vDSO callable directly from C by preserving RBX and taking leaf > from RCX. > > v28: > * Documented to Documentation/x86/sgx.rst how the kernel manages the > enclave ownership. > * Removed non-LC flow from sgx_einit(). > * Removed struct sgx_einittoken since only the size of the corresponding > microarchitectural structure is used in the series ATM. > > v27: > * Disallow RIE processes to use enclaves as there could a permission > conflict between VMA and enclave permissions. > * In the documentation, replace "grep /proc/cpuinfo" with > "grep sgx /proc/cpuinfo". > > v26: > * Fixed the commit author in "x86/sgx: Linux Enclave Driver", which was > changed in v25 by mistake. > * Addressed a bunch of grammar mistakes in sgx.rst (thanks Randy once > again for such a detailed feedback). > * Added back the MAINTAINERS update commit, which was mistakenly removed > in v25. > * EREMOVE's for SECS cannot be done while sanitizing an EPC section. The > CPU does not allow to remove a SECS page before all of its children have > been removed, and a child page can be in some other section than the one > currently being processed. Thus, removed special SECS processing from > sgx_sanitize_page() and instead put sections through it twice. In the > 2nd round the lists should only contain SECS pages. > > v
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-05-14 00:14, Jarkko Sakkinen wrote: > General question: maybe it would be easiest that I issue a pull request > once everyone feels that the series is ready to be pulled and stop sending > new versions of the series? Sounds good -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-30 10:23, Jarkko Sakkinen wrote: > On Thu, Apr 30, 2020 at 09:19:48AM +0200, Jethro Beekman wrote: >> On 2020-04-30 05:46, Jarkko Sakkinen wrote: >>> On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: >>>> On 2020-04-21 23:52, Jarkko Sakkinen wrote: >>>>> Intel(R) SGX is a set of CPU instructions that can be used by applications >>>>> to set aside private regions of code and data. The code outside the >>>>> enclave >>>>> is disallowed to access the memory inside the enclave by the CPU access >>>>> control. >>>>> >>>>> There is a new hardware unit in the processor called Memory Encryption >>>>> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define >>>>> one or many MEE regions that can hold enclave data by configuring them >>>>> with >>>>> PRMRR registers. >>>>> >>>>> The MEE automatically encrypts the data leaving the processor package to >>>>> the MEE regions. The data is encrypted using a random key whose life-time >>>>> is exactly one power cycle. >>>>> >>>>> The current implementation requires that the firmware sets >>>>> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can >>>>> decide what enclaves it wants run. The implementation does not create >>>>> any bottlenecks to support read-only MSRs later on. >>>>> >>>>> You can tell if your CPU supports SGX by looking into /proc/cpuinfo: >>>>> >>>>> cat /proc/cpuinfo | grep sgx >>>> >>>> Let's merge this. >>> >>> So can I tag reviewed-by's? >>> >> >> No, but you already have my tested-by's. >> >> If it helps I can try to review some patches, but 1) I know nothing >> about kernel coding guidelines and best practices and 2) I know little >> about most kernel internals, so I won't be able to review every patch. > > Ackd-by *acknowledges* that the patches work for you. I think that would > be then the correct choice for the driver patch and patches before that. > > Lets go with that if that is cool for you of course. > > Did you run the selftest only or possibly also some internal Fortanix > tests? > v29 patches 2 through 18: Acked-by: Jethro Beekman I only ran production SGX software. I didn't run the self test. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-30 05:46, Jarkko Sakkinen wrote: > On Wed, Apr 29, 2020 at 05:27:48PM +0200, Jethro Beekman wrote: >> On 2020-04-21 23:52, Jarkko Sakkinen wrote: >>> Intel(R) SGX is a set of CPU instructions that can be used by applications >>> to set aside private regions of code and data. The code outside the enclave >>> is disallowed to access the memory inside the enclave by the CPU access >>> control. >>> >>> There is a new hardware unit in the processor called Memory Encryption >>> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define >>> one or many MEE regions that can hold enclave data by configuring them with >>> PRMRR registers. >>> >>> The MEE automatically encrypts the data leaving the processor package to >>> the MEE regions. The data is encrypted using a random key whose life-time >>> is exactly one power cycle. >>> >>> The current implementation requires that the firmware sets >>> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can >>> decide what enclaves it wants run. The implementation does not create >>> any bottlenecks to support read-only MSRs later on. >>> >>> You can tell if your CPU supports SGX by looking into /proc/cpuinfo: >>> >>> cat /proc/cpuinfo | grep sgx >> >> Let's merge this. > > So can I tag reviewed-by's? > No, but you already have my tested-by's. If it helps I can try to review some patches, but 1) I know nothing about kernel coding guidelines and best practices and 2) I know little about most kernel internals, so I won't be able to review every patch. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v29 00/20] Intel SGX foundations
On 2020-04-21 23:52, Jarkko Sakkinen wrote: > Intel(R) SGX is a set of CPU instructions that can be used by applications > to set aside private regions of code and data. The code outside the enclave > is disallowed to access the memory inside the enclave by the CPU access > control. > > There is a new hardware unit in the processor called Memory Encryption > Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define > one or many MEE regions that can hold enclave data by configuring them with > PRMRR registers. > > The MEE automatically encrypts the data leaving the processor package to > the MEE regions. The data is encrypted using a random key whose life-time > is exactly one power cycle. > > The current implementation requires that the firmware sets > IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can > decide what enclaves it wants run. The implementation does not create > any bottlenecks to support read-only MSRs later on. > > You can tell if your CPU supports SGX by looking into /proc/cpuinfo: > > cat /proc/cpuinfo | grep sgx Let's merge this. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [RFC PATCH] Don't copy mark from encapsulated packet when routing VXLAN
On 2019-10-01 17:30, Roopa Prabhu wrote: > On Sun, Sep 29, 2019 at 11:27 PM Jethro Beekman wrote: >> >> When using rule-based routing to send traffic via VXLAN, a routing >> loop may occur. Say you have the following routing setup: >> >> ip rule add from all fwmark 0x2/0x2 lookup 2 >> ip route add table 2 default via 10.244.2.0 dev vxlan1 onlink >> >> The intention is to route packets with mark 2 through VXLAN, and >> this works fine. However, the current vxlan code copies the mark >> to the encapsulated packet. Immediately after egress on the VXLAN >> interface, the encapsulated packet is routed, with no opportunity >> to mangle the encapsulated packet. The mark is copied from the >> inner packet to the outer packet, and the same routing rule and >> table shown above will apply, resulting in ELOOP. >> >> This patch simply doesn't copy the mark from the encapsulated packet. >> I don't intend this code to land as is, but I want to start a >> discussion on how to make separate routing of VXLAN inner and >> encapsulated traffic easier. > > yeah, i think the patch as is will break users who use mark to > influence the underlay route lookup. > When you say the mark is copied into the packet, what exactly are you > seeing and where is the copy happening ? > Maybe the mark isn't actually copied? At least it's used in the route lookup as shown in the patch. -- Jethro Beekman | Fortanix > > >> >> Signed-off-by: Jethro Beekman >> --- >> drivers/net/vxlan.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c >> index 3d9bcc9..f9ed1b7 100644 >> --- a/drivers/net/vxlan.c >> +++ b/drivers/net/vxlan.c >> @@ -2236,7 +2236,6 @@ static struct rtable *vxlan_get_route(struct vxlan_dev >> *vxlan, struct net_device >> memset(, 0, sizeof(fl4)); >> fl4.flowi4_oif = oif; >> fl4.flowi4_tos = RT_TOS(tos); >> - fl4.flowi4_mark = skb->mark; >> fl4.flowi4_proto = IPPROTO_UDP; >> fl4.daddr = daddr; >> fl4.saddr = *saddr; >> @@ -2294,7 +2293,6 @@ static struct dst_entry *vxlan6_get_route(struct >> vxlan_dev *vxlan, >> fl6.daddr = *daddr; >> fl6.saddr = *saddr; >> fl6.flowlabel = ip6_make_flowinfo(RT_TOS(tos), label); >> - fl6.flowi6_mark = skb->mark; >> fl6.flowi6_proto = IPPROTO_UDP; >> fl6.fl6_dport = dport; >> fl6.fl6_sport = sport; >> -- >> 2.7.4 >> >> smime.p7s Description: S/MIME Cryptographic Signature
[RFC PATCH] Don't copy mark from encapsulated packet when routing VXLAN
When using rule-based routing to send traffic via VXLAN, a routing loop may occur. Say you have the following routing setup: ip rule add from all fwmark 0x2/0x2 lookup 2 ip route add table 2 default via 10.244.2.0 dev vxlan1 onlink The intention is to route packets with mark 2 through VXLAN, and this works fine. However, the current vxlan code copies the mark to the encapsulated packet. Immediately after egress on the VXLAN interface, the encapsulated packet is routed, with no opportunity to mangle the encapsulated packet. The mark is copied from the inner packet to the outer packet, and the same routing rule and table shown above will apply, resulting in ELOOP. This patch simply doesn't copy the mark from the encapsulated packet. I don't intend this code to land as is, but I want to start a discussion on how to make separate routing of VXLAN inner and encapsulated traffic easier. Signed-off-by: Jethro Beekman --- drivers/net/vxlan.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 3d9bcc9..f9ed1b7 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2236,7 +2236,6 @@ static struct rtable *vxlan_get_route(struct vxlan_dev *vxlan, struct net_device memset(, 0, sizeof(fl4)); fl4.flowi4_oif = oif; fl4.flowi4_tos = RT_TOS(tos); - fl4.flowi4_mark = skb->mark; fl4.flowi4_proto = IPPROTO_UDP; fl4.daddr = daddr; fl4.saddr = *saddr; @@ -2294,7 +2293,6 @@ static struct dst_entry *vxlan6_get_route(struct vxlan_dev *vxlan, fl6.daddr = *daddr; fl6.saddr = *saddr; fl6.flowlabel = ip6_make_flowinfo(RT_TOS(tos), label); - fl6.flowi6_mark = skb->mark; fl6.flowi6_proto = IPPROTO_UDP; fl6.fl6_dport = dport; fl6.fl6_sport = sport; -- 2.7.4 smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v2 1/2] mtd: spi-nor: intel-spi: support chips without software sequencer
On 2019-09-16 11:19, Mika Westerberg wrote: > On Mon, Sep 16, 2019 at 09:12:50AM +0000, Jethro Beekman wrote: >> On 2019-09-16 11:11, Mika Westerberg wrote: >>> Hi, >>> >>> On Sun, Sep 15, 2019 at 08:41:55PM +, Jethro Beekman wrote: >>>> Could someone please review this? >>>> >>>> On 2019-09-04 03:15, Jethro Beekman wrote: >>>>> Some flash controllers don't have a software sequencer. Avoid >>>>> configuring the register addresses for it, and double check >>>>> everywhere that its not accidentally trying to be used. >>> >>> All the supported types in intel_spi_init() set ->sregs so I don't see >>> how we could end up calling functions with that not set properly. Which >>> controller we are talking about here? CNL? >>> >> >> Yes, see 2/2. > > OK, thanks. Please mention that in the commit log as well. It seems obvious to me that the need for a patch may be further explained by the next patch in the patch set. -- Jethro Beekman | Fortanix
Re: [PATCH v2 1/2] mtd: spi-nor: intel-spi: support chips without software sequencer
On 2019-09-16 11:11, Mika Westerberg wrote: > Hi, > > On Sun, Sep 15, 2019 at 08:41:55PM +0000, Jethro Beekman wrote: >> Could someone please review this? >> >> On 2019-09-04 03:15, Jethro Beekman wrote: >>> Some flash controllers don't have a software sequencer. Avoid >>> configuring the register addresses for it, and double check >>> everywhere that its not accidentally trying to be used. > > All the supported types in intel_spi_init() set ->sregs so I don't see > how we could end up calling functions with that not set properly. Which > controller we are talking about here? CNL? > Yes, see 2/2. -- Jethro Beekman | Fortanix
Re: [PATCH v2 1/2] mtd: spi-nor: intel-spi: support chips without software sequencer
Could someone please review this?
On 2019-09-04 03:15, Jethro Beekman wrote:
> Some flash controllers don't have a software sequencer. Avoid
> configuring the register addresses for it, and double check
> everywhere that its not accidentally trying to be used.
>
> Every use of `sregs` is now guarded by a check of `sregs` or
> `swseq_reg`. The check might be done in the calling function.
>
> Signed-off-by: Jethro Beekman
> ---
> drivers/mtd/spi-nor/intel-spi.c | 23 ---
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index 1ccf23f..195cdca 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -187,12 +187,16 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
> dev_dbg(ispi->dev, "PR(%d)=0x%08x\n", i,
> readl(ispi->pregs + PR(i)));
>
> - value = readl(ispi->sregs + SSFSTS_CTL);
> - dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
> - dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
> - readl(ispi->sregs + PREOP_OPTYPE));
> - dev_dbg(ispi->dev, "OPMENU0=0x%08x\n", readl(ispi->sregs + OPMENU0));
> - dev_dbg(ispi->dev, "OPMENU1=0x%08x\n", readl(ispi->sregs + OPMENU1));
> + if (ispi->sregs) {
> + value = readl(ispi->sregs + SSFSTS_CTL);
> + dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
> + dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
> + readl(ispi->sregs + PREOP_OPTYPE));
> + dev_dbg(ispi->dev, "OPMENU0=0x%08x\n",
> + readl(ispi->sregs + OPMENU0));
> + dev_dbg(ispi->dev, "OPMENU1=0x%08x\n",
> + readl(ispi->sregs + OPMENU1));
> + }
>
> if (ispi->info->type == INTEL_SPI_BYT)
> dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
> @@ -367,6 +371,11 @@ static int intel_spi_init(struct intel_spi *ispi)
> !(uvscc & ERASE_64K_OPCODE_MASK))
> ispi->erase_64k = false;
>
> + if (ispi->sregs == NULL && (ispi->swseq_reg || ispi->swseq_erase)) {
> + dev_err(ispi->dev, "software sequencer not supported, but
> required\n");
> + return -EINVAL;
> + }
> +
> /*
>* Some controllers can only do basic operations using hardware
>* sequencer. All other operations are supposed to be carried out
> @@ -383,7 +392,7 @@ static int intel_spi_init(struct intel_spi *ispi)
> val = readl(ispi->base + HSFSTS_CTL);
> ispi->locked = !!(val & HSFSTS_CTL_FLOCKDN);
>
> - if (ispi->locked) {
> + if (ispi->locked && ispi->sregs) {
> /*
>* BIOS programs allowed opcodes and then locks down the
>* register. So read back what opcodes it decided to support.
>
[PATCH v2 1/2] mtd: spi-nor: intel-spi: support chips without software sequencer
Some flash controllers don't have a software sequencer. Avoid
configuring the register addresses for it, and double check
everywhere that its not accidentally trying to be used.
Every use of `sregs` is now guarded by a check of `sregs` or
`swseq_reg`. The check might be done in the calling function.
Signed-off-by: Jethro Beekman
---
drivers/mtd/spi-nor/intel-spi.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 1ccf23f..195cdca 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -187,12 +187,16 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
dev_dbg(ispi->dev, "PR(%d)=0x%08x\n", i,
readl(ispi->pregs + PR(i)));
- value = readl(ispi->sregs + SSFSTS_CTL);
- dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
- dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
- readl(ispi->sregs + PREOP_OPTYPE));
- dev_dbg(ispi->dev, "OPMENU0=0x%08x\n", readl(ispi->sregs + OPMENU0));
- dev_dbg(ispi->dev, "OPMENU1=0x%08x\n", readl(ispi->sregs + OPMENU1));
+ if (ispi->sregs) {
+ value = readl(ispi->sregs + SSFSTS_CTL);
+ dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
+ dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
+ readl(ispi->sregs + PREOP_OPTYPE));
+ dev_dbg(ispi->dev, "OPMENU0=0x%08x\n",
+ readl(ispi->sregs + OPMENU0));
+ dev_dbg(ispi->dev, "OPMENU1=0x%08x\n",
+ readl(ispi->sregs + OPMENU1));
+ }
if (ispi->info->type == INTEL_SPI_BYT)
dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
@@ -367,6 +371,11 @@ static int intel_spi_init(struct intel_spi *ispi)
!(uvscc & ERASE_64K_OPCODE_MASK))
ispi->erase_64k = false;
+ if (ispi->sregs == NULL && (ispi->swseq_reg || ispi->swseq_erase)) {
+ dev_err(ispi->dev, "software sequencer not supported, but
required\n");
+ return -EINVAL;
+ }
+
/*
* Some controllers can only do basic operations using hardware
* sequencer. All other operations are supposed to be carried out
@@ -383,7 +392,7 @@ static int intel_spi_init(struct intel_spi *ispi)
val = readl(ispi->base + HSFSTS_CTL);
ispi->locked = !!(val & HSFSTS_CTL_FLOCKDN);
- if (ispi->locked) {
+ if (ispi->locked && ispi->sregs) {
/*
* BIOS programs allowed opcodes and then locks down the
* register. So read back what opcodes it decided to support.
--
2.7.4
[PATCH v2 2/2] mtd: spi-nor: intel-spi: add support for Intel Cannon Lake SPI flash
Now that SPI flash controllers without a software sequencer are
supported, it's trivial to add support for CNL and its PCI ID.
Values from
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/300-series-chipset-pch-datasheet-vol-2.pdf
Signed-off-by: Jethro Beekman
---
drivers/mtd/spi-nor/intel-spi-pci.c | 5 +
drivers/mtd/spi-nor/intel-spi.c | 11 +++
include/linux/platform_data/intel-spi.h | 1 +
3 files changed, 17 insertions(+)
diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
b/drivers/mtd/spi-nor/intel-spi-pci.c
index b83c4ab6..195a09d 100644
--- a/drivers/mtd/spi-nor/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/intel-spi-pci.c
@@ -20,6 +20,10 @@ static const struct intel_spi_boardinfo bxt_info = {
.type = INTEL_SPI_BXT,
};
+static const struct intel_spi_boardinfo cnl_info = {
+ .type = INTEL_SPI_CNL,
+};
+
static int intel_spi_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -67,6 +71,7 @@ static const struct pci_device_id intel_spi_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x4b24), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa1a4), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa224), (unsigned long)_info },
+ { PCI_VDEVICE(INTEL, 0xa324), (unsigned long)_info },
{ },
};
MODULE_DEVICE_TABLE(pci, intel_spi_pci_ids);
diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 195cdca..91b7851 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -108,6 +108,10 @@
#define BXT_FREG_NUM 12
#define BXT_PR_NUM 6
+#define CNL_PR 0x84
+#define CNL_FREG_NUM 6
+#define CNL_PR_NUM 5
+
#define LVSCC 0xc4
#define UVSCC 0xc8
#define ERASE_OPCODE_SHIFT 8
@@ -344,6 +348,13 @@ static int intel_spi_init(struct intel_spi *ispi)
ispi->erase_64k = true;
break;
+ case INTEL_SPI_CNL:
+ ispi->sregs = NULL;
+ ispi->pregs = ispi->base + CNL_PR;
+ ispi->nregions = CNL_FREG_NUM;
+ ispi->pr_num = CNL_PR_NUM;
+ break;
+
default:
return -EINVAL;
}
diff --git a/include/linux/platform_data/intel-spi.h
b/include/linux/platform_data/intel-spi.h
index ebb4f33..7f53a5c 100644
--- a/include/linux/platform_data/intel-spi.h
+++ b/include/linux/platform_data/intel-spi.h
@@ -13,6 +13,7 @@ enum intel_spi_type {
INTEL_SPI_BYT = 1,
INTEL_SPI_LPT,
INTEL_SPI_BXT,
+ INTEL_SPI_CNL,
};
/**
--
2.7.4
[PATCH v2 0/2] Support for Intel Cannon Lake SPI flash
v2 changes: * Fix whitespace. * Link to datasheet. Jethro Beekman (2): mtd: spi-nor: intel-spi: support chips without software sequencer mtd: spi-nor: intel-spi: add support for Intel Cannon Lake SPI flash drivers/mtd/spi-nor/intel-spi-pci.c | 5 + drivers/mtd/spi-nor/intel-spi.c | 34 ++--- include/linux/platform_data/intel-spi.h | 1 + 3 files changed, 33 insertions(+), 7 deletions(-) -- 2.7.4
Re: [PATCH 2/2] mtd: spi-nor: intel-spi: add support for Intel Cannon Lake SPI flash
On 2019-08-31 06:36, Mika Westerberg wrote: Looks like some white space damage. There are couple of similar below as well. Oops. I will fix this in a v2 or resend later. + ispi->sregs = NULL; + ispi->pregs = ispi->base + CNL_PR; + ispi->nregions = CNL_FREG_NUM; + ispi->pr_num = CNL_PR_NUM; Does CNL really have a different number of PR and FR regions than the previous generations? I'm using this as a reference: https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/300-series-chipset-pch-datasheet-vol-2.pdf . If you have more accurate information, please let me know. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
[PATCH 2/2] mtd: spi-nor: intel-spi: add support for Intel Cannon Lake SPI flash
(apologies, resending without S/MIME signature)
Now that SPI flash controllers without a software sequencer are
supported, it's trivial to add support for CNL and its PCI ID.
Signed-off-by: Jethro Beekman
---
drivers/mtd/spi-nor/intel-spi-pci.c | 5 +
drivers/mtd/spi-nor/intel-spi.c | 11 +++
include/linux/platform_data/intel-spi.h | 1 +
3 files changed, 17 insertions(+)
diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
b/drivers/mtd/spi-nor/intel-spi-pci.c
index b83c4ab6..195a09d 100644
--- a/drivers/mtd/spi-nor/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/intel-spi-pci.c
@@ -20,6 +20,10 @@ static const struct intel_spi_boardinfo bxt_info = {
.type = INTEL_SPI_BXT,
};
+static const struct intel_spi_boardinfo cnl_info = {
+ .type = INTEL_SPI_CNL,
+};
+
static int intel_spi_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -67,6 +71,7 @@ static const struct pci_device_id intel_spi_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x4b24), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa1a4), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa224), (unsigned long)_info },
+ { PCI_VDEVICE(INTEL, 0xa324), (unsigned long)_info },
{ },
};
MODULE_DEVICE_TABLE(pci, intel_spi_pci_ids);
diff --git a/drivers/mtd/spi-nor/intel-spi.c
b/drivers/mtd/spi-nor/intel-spi.c
index 195cdca..91b7851 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -108,6 +108,10 @@
#define BXT_FREG_NUM 12
#define BXT_PR_NUM 6
+#define CNL_PR 0x84
+#define CNL_FREG_NUM 6
+#define CNL_PR_NUM 5
+
#define LVSCC0xc4
#define UVSCC0xc8
#define ERASE_OPCODE_SHIFT 8
@@ -344,6 +348,13 @@ static int intel_spi_init(struct intel_spi *ispi)
ispi->erase_64k = true;
break;
+case INTEL_SPI_CNL:
+ ispi->sregs = NULL;
+ ispi->pregs = ispi->base + CNL_PR;
+ ispi->nregions = CNL_FREG_NUM;
+ ispi->pr_num = CNL_PR_NUM;
+ break;
+
default:
return -EINVAL;
}
diff --git a/include/linux/platform_data/intel-spi.h
b/include/linux/platform_data/intel-spi.h
index ebb4f33..7f53a5c 100644
--- a/include/linux/platform_data/intel-spi.h
+++ b/include/linux/platform_data/intel-spi.h
@@ -13,6 +13,7 @@ enum intel_spi_type {
INTEL_SPI_BYT = 1,
INTEL_SPI_LPT,
INTEL_SPI_BXT,
+ INTEL_SPI_CNL,
};
/**
--
2.7.4
[PATCH 2/2] mtd: spi-nor: intel-spi: add support for Intel Cannon Lake SPI flash
Now that SPI flash controllers without a software sequencer are
supported, it's trivial to add support for CNL and its PCI ID.
Signed-off-by: Jethro Beekman
---
drivers/mtd/spi-nor/intel-spi-pci.c | 5 +
drivers/mtd/spi-nor/intel-spi.c | 11 +++
include/linux/platform_data/intel-spi.h | 1 +
3 files changed, 17 insertions(+)
diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
b/drivers/mtd/spi-nor/intel-spi-pci.c
index b83c4ab6..195a09d 100644
--- a/drivers/mtd/spi-nor/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/intel-spi-pci.c
@@ -20,6 +20,10 @@ static const struct intel_spi_boardinfo bxt_info = {
.type = INTEL_SPI_BXT,
};
+static const struct intel_spi_boardinfo cnl_info = {
+ .type = INTEL_SPI_CNL,
+};
+
static int intel_spi_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -67,6 +71,7 @@ static const struct pci_device_id intel_spi_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x4b24), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa1a4), (unsigned long)_info },
{ PCI_VDEVICE(INTEL, 0xa224), (unsigned long)_info },
+ { PCI_VDEVICE(INTEL, 0xa324), (unsigned long)_info },
{ },
};
MODULE_DEVICE_TABLE(pci, intel_spi_pci_ids);
diff --git a/drivers/mtd/spi-nor/intel-spi.c
b/drivers/mtd/spi-nor/intel-spi.c
index 195cdca..91b7851 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -108,6 +108,10 @@
#define BXT_FREG_NUM 12
#define BXT_PR_NUM 6
+#define CNL_PR0x84
+#define CNL_FREG_NUM 6
+#define CNL_PR_NUM 5
+
#define LVSCC 0xc4
#define UVSCC 0xc8
#define ERASE_OPCODE_SHIFT 8
@@ -344,6 +348,13 @@ static int intel_spi_init(struct intel_spi *ispi)
ispi->erase_64k = true;
break;
+ case INTEL_SPI_CNL:
+ ispi->sregs = NULL;
+ ispi->pregs = ispi->base + CNL_PR;
+ ispi->nregions = CNL_FREG_NUM;
+ ispi->pr_num = CNL_PR_NUM;
+ break;
+
default:
return -EINVAL;
}
diff --git a/include/linux/platform_data/intel-spi.h
b/include/linux/platform_data/intel-spi.h
index ebb4f33..7f53a5c 100644
--- a/include/linux/platform_data/intel-spi.h
+++ b/include/linux/platform_data/intel-spi.h
@@ -13,6 +13,7 @@ enum intel_spi_type {
INTEL_SPI_BYT = 1,
INTEL_SPI_LPT,
INTEL_SPI_BXT,
+ INTEL_SPI_CNL,
};
/**
--
2.7.4
smime.p7s
Description: S/MIME Cryptographic Signature
[PATCH 1/2] mtd: spi-nor: intel-spi: support chips without software sequencer
Some flash controllers don't have a software sequencer. Avoid
configuring the register addresses for it, and double check
everywhere that its not accidentally trying to be used.
Every use of `sregs` is now guarded by a check of `sregs` or
`swseq_reg`. The check might be done in the calling function.
Signed-off-by: Jethro Beekman
---
drivers/mtd/spi-nor/intel-spi.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/spi-nor/intel-spi.c
b/drivers/mtd/spi-nor/intel-spi.c
index 1ccf23f..195cdca 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -187,12 +187,16 @@ static void intel_spi_dump_regs(struct intel_spi
*ispi)
dev_dbg(ispi->dev, "PR(%d)=0x%08x\n", i,
readl(ispi->pregs + PR(i)));
- value = readl(ispi->sregs + SSFSTS_CTL);
- dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
- dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
- readl(ispi->sregs + PREOP_OPTYPE));
- dev_dbg(ispi->dev, "OPMENU0=0x%08x\n", readl(ispi->sregs + OPMENU0));
- dev_dbg(ispi->dev, "OPMENU1=0x%08x\n", readl(ispi->sregs + OPMENU1));
+ if (ispi->sregs) {
+ value = readl(ispi->sregs + SSFSTS_CTL);
+ dev_dbg(ispi->dev, "SSFSTS_CTL=0x%08x\n", value);
+ dev_dbg(ispi->dev, "PREOP_OPTYPE=0x%08x\n",
+ readl(ispi->sregs + PREOP_OPTYPE));
+ dev_dbg(ispi->dev, "OPMENU0=0x%08x\n",
+ readl(ispi->sregs + OPMENU0));
+ dev_dbg(ispi->dev, "OPMENU1=0x%08x\n",
+ readl(ispi->sregs + OPMENU1));
+ }
if (ispi->info->type == INTEL_SPI_BYT)
dev_dbg(ispi->dev, "BCR=0x%08x\n", readl(ispi->base + BYT_BCR));
@@ -367,6 +371,11 @@ static int intel_spi_init(struct intel_spi *ispi)
!(uvscc & ERASE_64K_OPCODE_MASK))
ispi->erase_64k = false;
+ if (ispi->sregs == NULL && (ispi->swseq_reg || ispi->swseq_erase)) {
+ dev_err(ispi->dev, "software sequencer not supported, but
required\n");
+ return -EINVAL;
+ }
+
/*
* Some controllers can only do basic operations using hardware
* sequencer. All other operations are supposed to be carried out
@@ -383,7 +392,7 @@ static int intel_spi_init(struct intel_spi *ispi)
val = readl(ispi->base + HSFSTS_CTL);
ispi->locked = !!(val & HSFSTS_CTL_FLOCKDN);
- if (ispi->locked) {
+ if (ispi->locked && ispi->sregs) {
/*
* BIOS programs allowed opcodes and then locks down the
* register. So read back what opcodes it decided to support.
--
2.7.4
Re: [PATCH v21 16/28] x86/sgx: Add the Linux SGX Enclave Driver
ECPM permissions are mentioned in SDM EADD instruction operation. PTE I don't know. -- Jethro Beekman | Fortanix On 2019-08-07 08:17, Jarkko Sakkinen wrote: On Wed, Aug 07, 2019 at 06:15:34PM +0300, Jarkko Sakkinen wrote: On Mon, Jul 29, 2019 at 11:17:57AM +, Ayoun, Serge wrote: + /* TCS pages need to be RW in the PTEs, but can be 0 in the EPCM. */ + if ((secinfo.flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS) + prot |= PROT_READ | PROT_WRITE; For TCS pages you add both RD and WR maximum protection bits. For the enclave to be able to run, user mode will have to change the "vma->vm_flags" from PROT_NONE to PROT_READ | PROT_WRITE (otherwise eenter fails). This is exactly what your selftest does. Recap where the TCS requirements came from? Why does it need RW in PTEs and can be 0 in the EPCM? The comment should explain it rather leave it as a claim IMHO. I mean after ~3 weeks of not looking into SGX (because being on vacation etc.) I cannot remember details of this. /Jarkko smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v21 00/28] Intel SGX foundations
On 2019-07-14 07:36, Jarkko Sakkinen wrote: On Sat, Jul 13, 2019 at 08:07:36PM +0300, Jarkko Sakkinen wrote: v21: * Check on mmap() that the VMA does cover an area that does not have enclave pages. Only mapping with PROT_NONE can do that to reserve initial address space for an enclave. * Check om mmap() and mprotect() that the VMA permissions do not surpass the enclave permissions. * Remove two refcounts from vma_close(): mm_list and encl->refcount. Enclave refcount is only need for swapper/enclave sync and we can remove mm_list refcount by destroying mm_struct when the process is closed. By not having vm_close() the Linux MM can merge VMAs. * Do not naturally align MAP_FIXED address. * Numerous small fixes and clean ups. * Use SRCU for synchronizing the list of mm_struct's. * Move to stack based call convention in the vDSO. I forgot something: * CONFIG_INTEL_SGX_DRIVER is not bistate i.e. no more LKM support. It is still useful to have the compile-time option because VM host does not need to have it enabled. Now sgx_init() calls explicitly sgx_drv_init(). In addition, platform driver has been ripped a way because we no longer need ACPI hotplug. In effect, the device is now parentless. I think you also missed in the changelog that you're now checking page permissions in EADD. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v21 18/28] x86/sgx: Add swapping code to the core and SGX driver
On 2019-07-13 10:07, Jarkko Sakkinen wrote:
Because the kernel is untrusted, swapping pages in/out of the Enclave
Page Cache (EPC) has specialized requirements:
* The kernel cannot directly access EPC memory, i.e. cannot copy data
to/from the EPC.
* To evict a page from the EPC, the kernel must "prove" to hardware that
are no valid TLB entries for said page since a stale TLB entry would
allow an attacker to bypass SGX access controls.
* When loading a page back into the EPC, hardware must be able to verify
the integrity and freshness of the data.
* When loading an enclave page, e.g. regular pages and Thread Control
Structures (TCS), hardware must be able to associate the page with a
Secure Enclave Control Structure (SECS).
To satisfy the above requirements, the CPU provides dedicated ENCLS
functions to support paging data in/out of the EPC:
* EBLOCK: Mark a page as blocked in the EPC Map (EPCM). Attempting
to access a blocked page that misses the TLB will fault.
* ETRACK: Activate blocking tracking. Hardware verifies that all
translations for pages marked as "blocked" have been flushed
from the TLB.
* EPA: Add version array page to the EPC. As the name suggests, a
VA page is an 512-entry array of version numbers that are
used to uniquely identify pages evicted from the EPC.
* EWB: Write back a page from EPC to memory, e.g. RAM. Software
must supply a VA slot, memory to hold the a Paging Crypto
Metadata (PCMD) of the page and obviously backing for the
evicted page.
* ELD{B,U}: Load a page in {un}blocked state from memory to EPC. The
driver only uses the ELDU variant as there is no use case
for loading a page as "blocked" in a bare metal environment.
To top things off, all of the above ENCLS functions are subject to
strict concurrency rules, e.g. many operations will #GP fault if two
or more operations attempt to access common pages/structures.
To put it succinctly, paging in/out of the EPC requires coordinating
with the SGX driver where all of an enclave's tracking resides. But,
simply shoving all reclaim logic into the driver is not desirable as
doing so has unwanted long term implications:
* Oversubscribing EPC to KVM guests, i.e. virtualizing SGX in KVM and
swapping a guest's EPC pages (without the guest's cooperation) needs
the same high level flows for reclaim but has painfully different
semantics in the details.
* Accounting EPC, i.e. adding an EPC cgroup controller, is desirable
as EPC is effectively a specialized memory type and even more scarce
than system memory. Providing a single touchpoint for EPC accounting
regardless of end consumer greatly simplifies the EPC controller.
* Allowing the userspace-facing driver to be built as a loaded module
is desirable, e.g. for debug, testing and development. The cgroup
infrastructure does not support dependencies on loadable modules.
* Separating EPC swapping from the driver once it has been tightly
coupled to the driver is non-trivial (speaking from experience).
Some of these points seem stale now.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 2019-05-21 08:19, Jarkko Sakkinen wrote: We could even disallow mmap() before EINIT done. This would be extremely annoying in software because now you have to save the all the page permissions somewhere between EADD and mprotect. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-05-10 11:56, Xing, Cedric wrote: Hi Jethro, ELF files are explicitly designed such that you can map them (with mmap) in 4096-byte chunks. However, sometimes there's overlap and you will sometimes see that a particular offset is mapped twice because the first half of the page in the file belongs to an RX range and the second half to an R-only range. Also, ELF files don't (normally) describe stack, heap, etc. which you do need for enclaves. You have probably misread my email. By mmap(), I meant the enclave file would be mapped via *multiple* mmap() calls, in the same way as what dlopen() would do in loading regular shared object. The intention here is to make the enclave file subject to the same checks as regular shared objects. No, I didn't misread your email. My original point still stands: requiring that an enclave's memory is created from one or more mmap calls of a file puts significant restrictions on the enclave's on-disk representation. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-05-10 10:54, Dave Hansen wrote: On 5/10/19 10:37 AM, Jethro Beekman wrote: It does assume a specific format, namely, that the memory layout (including page types/permissions) of the enclave can be represented in a "flat file" on disk, or at least that the enclave memory contents consist of 4096-byte chunks in that file. I _think_ Cedric's point is that, to the kernel, /lib/x86_64-linux-gnu/libc.so.6 is a "flat file" because the kernel doesn't have any part in parsing the executable format of a shared library. I actually don't know how it works, though. Do we just just trust that the userspace parsing of the .so format is correct? Do we just assume that any part of a file passing IMA checks can be PROT_EXEC? ELF files are explicitly designed such that you can map them (with mmap) in 4096-byte chunks. However, sometimes there's overlap and you will sometimes see that a particular offset is mapped twice because the first half of the page in the file belongs to an RX range and the second half to an R-only range. Also, ELF files don't (normally) describe stack, heap, etc. which you do need for enclaves. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-05-10 10:23, Xing, Cedric wrote: ... I think an alternative could be to treat an enclave file as a regular shared object so all IMA/LSM checks could be triggered/performed as part of the loading process, then let the driver "copy" those pages to EPC. ... If compared to the idea of "enclave loader inside kernel", I think this alternative is much simpler and more flexible. In particular, * It requires minimal change to the driver - just take EPCM permissions from source pages' VMA instead of from ioctl parameter. * It requires little change to user mode enclave loader - just mmap() enclave file in the same way as dlopen() would do, then all IMA/LSM checks applicable to shared objects will be triggered/performed transparently. * It doesn't assume/tie to specific enclave formats. It does assume a specific format, namely, that the memory layout (including page types/permissions) of the enclave can be represented in a "flat file" on disk, or at least that the enclave memory contents consist of 4096-byte chunks in that file. Of the formats I have described in my other email, the only format that satisfies this property is the "CPU-native (instruction stream)" format which is not in use today. The ELF/PE formats in use today don't satisfy this property as the files don't contain the TCS contents. The SGXS format doesn't satisfy this property because the enclave memory is represented with 256-byte chunks. * It is extensible - Today every regular page within a process is allowed implicitly to be the source for an EPC page. In future, if at all desirable/necessary, IMA/LSM could be extended to leave a flag somewhere (e.g. in VMA) to indicate explicitly whether a regular page (or range) could be a source for EPC. Then SGX specific hooks/policies could be supported easily. How do you think? -Cedric -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 16/28] x86/sgx: Add provisioning
On 2019-04-17 03:39, Jarkko Sakkinen wrote: diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 7bf627ac4958..3b80acde8671 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -16,6 +16,8 @@ _IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page) #define SGX_IOC_ENCLAVE_INIT \ _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init) +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \ + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute) Need to update Documentation/ioctl/ioctl-number.txt as well -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 15/28] x86/sgx: Add the Linux SGX Enclave Driver
On 2019-04-23 17:26, Sean Christopherson wrote: On Tue, Apr 23, 2019 at 11:29:24PM +, Jethro Beekman wrote: On 2019-04-22 14:58, Sean Christopherson wrote: Now that the core SGX code is approaching stability, I'd like to start sending RFCs for the EPC virtualization and KVM bits to hash out that side of things. The ACPI crud is the last chunk of code that would require non-trivial changes to the core SGX code for the proposed virtualization implementation. I'd strongly prefer to get it out of the way before sending the KVM RFCs. What kind of changes? Wouldn't KVM just be another consumer of the same API used by the driver? Nope, userspace "only" needs to be able to mmap() arbitrary chunks of EPC. I don't think this is sufficient. Don't you need enclave tracking in order to support paging? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v20 15/28] x86/sgx: Add the Linux SGX Enclave Driver
On 2019-04-22 14:58, Sean Christopherson wrote:
+Cc Jethro
On Wed, Apr 17, 2019 at 01:39:25PM +0300, Jarkko Sakkinen wrote:
Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
can be used by applications to set aside private regions of code and
data. The code outside the enclave is disallowed to access the memory
inside the enclave by the CPU access control.
This commit adds the Linux SGX Enclave Driver that provides an ioctl API
to manage enclaves. The address range for an enclave, commonly referred
as ELRANGE in the documentation (e.g. Intel SDM), is reserved with
mmap() against /dev/sgx/enclave. After that a set ioctls is used to
build the enclave to the ELRANGE.
Signed-off-by: Jarkko Sakkinen
Co-developed-by: Sean Christopherson
Signed-off-by: Sean Christopherson
Co-developed-by: Serge Ayoun
Signed-off-by: Serge Ayoun
Co-developed-by: Shay Katz-zamir
Signed-off-by: Shay Katz-zamir
Co-developed-by: Suresh Siddha
Signed-off-by: Suresh Siddha
---
...
+#ifdef CONFIG_ACPI
+static struct acpi_device_id sgx_device_ids[] = {
+ {"INT0E0C", 0},
+ {"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, sgx_device_ids);
+#endif
+
+static struct platform_driver sgx_drv = {
+ .probe = sgx_drv_probe,
+ .remove = sgx_drv_remove,
+ .driver = {
+ .name = "sgx",
+ .acpi_match_table = ACPI_PTR(sgx_device_ids),
+ },
+};
Where do we stand on removing the ACPI and platform_driver dependencies?
Can we get rid of them sooner rather than later?
You know my position on this...
https://www.spinics.net/lists/linux-sgx/msg00624.html . I don't really
have any new arguments.
Considering the amount of planned changes for the driver post-merge, I
think it's crucial that the driver part can be swapped out with
alternative implementations.
Now that the core SGX code is approaching stability, I'd like to start
sending RFCs for the EPC virtualization and KVM bits to hash out that side
of things. The ACPI crud is the last chunk of code that would require
non-trivial changes to the core SGX code for the proposed virtualization
implementation. I'd strongly prefer to get it out of the way before
sending the KVM RFCs.
What kind of changes? Wouldn't KVM just be another consumer of the same
API used by the driver?
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-22 09:26, Andy Lutomirski wrote: On Apr 19, 2019, at 2:56 PM, Jethro Beekman wrote: This works fine with v20 as-is. However, consider the equivalent of the PT-based flow: mmap(PROT_READ|PROT_EXEC) ioctl(EADD) <-- no error! Indeed! It's not me that's working around the LSM, it's the SGX driver! It's writing to memory that's not marked writable! The fundamental issue here is that the SGX instruction set has several instructions that bypass the page table permission bits, and this is (naturally) confusing to any kind of reference monitor like the LSM framework. You can come up with similar scenarios that involve PROT_READ|PROT_WRITE|PROT_EXEC or ptrace(PTRACE_POKETEXT). So, clearly, the proper way to fix this failure of complete mediation is by enforcing appropriate page-table permissions even on the SGX instructions that don't do it themselves. Just make any implicit memory access look like a regular memory access and now everyone is on the same page (pun intended). I agree that we should do this. But then what? Then, we have a minimum viable SGX implementation that doesn't make things worse than they are today from a userspace code loading/LSM perspective. People without LSMs can use SGX and people with LSMs are not more vulnerable than before. I agree that we should do something along the following lines... So I think we need a better EADD ioctl that explicitly does work on PROT_READ|PROT_EXEC enclave memory but makes up for by validating the *source* of the data. The effect will be similar to mapping a labeled, appraised, etc file as PROT_EXEC. ... but I don't see why this would need to be in the initial patch set. We need to take some time to explore the design space here (see additional comments below), and I don't think it's necessary to wait for it. Maybe, in extreme pseudocode: fd = open(“/dev/sgx/enclave”); ioctl(fd, SGX_CREATE_FROM_FILE, file_fd); // fd now inherits the LSM label from the file, or is otherwise marked. mmap(fd, PROT_READ|PROT_EXEC, ...); I suppose that an alternative would be to delegate all the EADD calls to a privileged daemon, but that’s nasty. What file format should this be in? I have worked with several different binary enclave formats and I don't really like any of them. # ELF Pros: * People know about ELF. * Allows storing additional metadata that is only used by userspace, not the enclave itself. Cons: * ELF generally loads all kinds of stuff in memory that is not necessary for enclaves, such as the ELF header. * Special tools are needed to calculate ENCLAVEHASH, for signing & verification. * All tools need to agree on the exact transformation. * Unclear how to specify things such as: which 256-byte chunks of memory should be measured, heap, TCS pages, stacks, SSAs, etc. Questions: * If using ELF, should this be the same format that the Intel Linux SDK uses (not documented, but source code is available) or something newly standardized? # PE (Windows Intel SDK format) Andy suggested this in another email. I'm not sure why exactly? Pros: * Used by Windows enclaves? * Allows storing additional metadata that is only used by userspace, not the enclave itself. Cons: * The format is not documented. I did some RE on this format a long time ago. See https://github.com/fortanix/rust-sgx/blob/master/doc/WINTEL-SGX-ABI.md and https://github.com/fortanix/rust-sgx/blob/master/sgxs-tools/src/bin/isgx-pe2sgx.rs. * PE is not really used on Linux. * All same cons as ELF above. # CPU-native (hash stream) "SGXS" The security properties of an enclave are completely defined by the hash that's calculated by the processor while loading the enclave. The exact hashed data is what I call the "SGX stream" format (SGXS). This is fully described by the Intel SDM. I've written down some notes about this at https://github.com/fortanix/rust-sgx/blob/master/doc/SGXS.md. That document also defines a notion of canonicality for streams. You can ignore the section on "Enhanced SGXS", which is a failed experiment. Pros: * Computing ENCLAVEHASH is a simple SHA256 of the file. * No complex transformations needed to load enclave. Cons: * No way to specify memory contents of non-measured memory. * No space for non-enclave metadata (including SIGSTRUCT). * Not a standard format for transporting binaries. # CPU-native (instruction stream) An enclave's memory contents is fully defined by the set of ECREATE/EADD/EEXTEND/EINIT instructions that the OS needs to execute. One could envision a format that describes exactly those instructions. One difference with the SGXS format described above is that the enclave memory is described as part of EADD, not EEXTEND. This allows including specific values for non-measured memory. Pros: * No complex transformations needed to load enclave. * Obvious place to store SIGSTRUCT. Cons: * Special tools are needed to calculate ENCLAVEHASH, for signing &
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 14:34, Thomas Gleixner wrote: > And how so? You create writeable AND executable memory. That's a nono and > you can argue in circles, that's not going to change with any of your > proposed changes. On 2019-04-19 14:38, Thomas Gleixner wrote: > You are working around LSM nothing else and that's just not going to fly. Based on your comments, I'm still unsure if we're on the same page with regards to what I'm proposing. Here's a regular non-SGX flow that LSM would likely prevent: mmap(PROT_READ|PROT_WRITE) memcpy() mmap(PROT_READ|PROT_EXEC) <-- denied by LSM Or just something based on regular PT permissions: mmap(PROT_READ|PROT_EXEC) memcpy() <-- SIGSEGV Now, the equivalent for SGX: mmap(PROT_READ|PROT_WRITE) ioctl(EADD) mmap(PROT_READ|PROT_EXEC) <-- denied by LSM This works fine with v20 as-is. However, consider the equivalent of the PT-based flow: mmap(PROT_READ|PROT_EXEC) ioctl(EADD) <-- no error! It's not me that's working around the LSM, it's the SGX driver! It's writing to memory that's not marked writable! The fundamental issue here is that the SGX instruction set has several instructions that bypass the page table permission bits, and this is (naturally) confusing to any kind of reference monitor like the LSM framework. You can come up with similar scenarios that involve PROT_READ|PROT_WRITE|PROT_EXEC or ptrace(PTRACE_POKETEXT). So, clearly, the proper way to fix this failure of complete mediation is by enforcing appropriate page-table permissions even on the SGX instructions that don't do it themselves. Just make any implicit memory access look like a regular memory access and now everyone is on the same page (pun intended). -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 14:31, Andy Lutomirski wrote: > I do think we need to follow LSM rules. But my bigger point is that there > are policies that don’t allow JIT at all. I think we should arrange the SGX > API so it’s still usable when such a policy is in effect. I don't think we need to arrange that right now. This patch set needs to be merged after more than 2 years of development. I'd like to avoid introducing any more big changes. Let's just do what I described to make LSM not broken, which is a minimal change to the current approach. We can adjust the API later to support the use case you describe. -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
Jethro Beekman | Fortanix On 2019-04-19 14:15, Andy Lutomirski wrote: > > > > >> On Apr 19, 2019, at 1:54 PM, Jethro Beekman wrote: >> >>> On 2019-04-19 13:50, Thomas Gleixner wrote: >>>> On Fri, 19 Apr 2019, Jethro Beekman wrote: >>>>> On 2019-04-19 13:39, Thomas Gleixner wrote: >>>>>> On Fri, 19 Apr 2019, Jethro Beekman wrote: >>>>>> >>>>>>> On 2019-04-19 08:27, Andy Lutomirski wrote: >>>>>>> There are many, >>>>>>> many Linux systems that enforce a policy that *all* executable text >>>>>>> needs to come from a verified source. On these systems, you can't >>>>>>> mmap some writable memory, write to it, and then change it to >>>>>>> executable. >>>>>> >>>>>> How is this implemented on those systems? AFAIK there's no kernel config >>>>>> option that changes the semantics of mmap as you describe. >>>>> >>>>> That has nothing to do with mmap() semantics. You mmap() writeable memory >>>>> and then you change the permissions via mprotect(). mprotect() calls into >>>>> LSM and depending on policy and security model this will reject the >>>>> request. >>>>> >>>>> Andy was pointing out that the SGX ioctl bypasses the LSM mechanics which >>>>> is obviously a bad thing. >>>> >>>> We could modify the driver such that when you call ioctl EADD, the page >>>> table permissions need to be the PAGEINFO.SECINFO.FLAGS | PROT_WRITE, >>>> otherwise you get EPERM or so. After EADD, if you want, you can restrict >>>> the page table permissions again using mprotect but the page table >>>> permissions don't really matter for SGX. >>> >>> And the point of that is? That you still can cirumvent LSM for feeding >>> executable code into SGX. >> >> How? LSM would see that you're trying to map a page RWX so you can do >> your ioctl? > > With plain mmap() + mprotect(), the LSM will prevent you from making memory > that *was* writable executable. This is by design and SELinux supports it. > I don’t remember the name of the associated SELinux permission off the top of > my head. > > If we start enforcing equivalent rules on SGX, then the current API will > simply not allow enclaves to be loaded — no matter how you slice it, loading > an enclave with the current API is indistinguishable from making arbitrary > data executable. Yes this is exactly what I intended here: a very simple change that stops SGX from confusing LSM. Just by enforcing that everything that looks like a memory write (EADD, EAUG, EDBGWR, etc.) actually requires write permissions, reality and LSM should be on the same page. If you want to go further and actually allow this behavior when your LSM would otherwise prohibit it, presumably the same workarounds that exist for JITs can be used for SGX. -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 13:46, Jethro Beekman wrote: > On 2019-04-19 13:39, Thomas Gleixner wrote: >> On Fri, 19 Apr 2019, Jethro Beekman wrote: >> >>> On 2019-04-19 08:27, Andy Lutomirski wrote: >>>> There are many, >>>> many Linux systems that enforce a policy that *all* executable text >>>> needs to come from a verified source. On these systems, you can't >>>> mmap some writable memory, write to it, and then change it to >>>> executable. >>> >>> How is this implemented on those systems? AFAIK there's no kernel config >>> option that changes the semantics of mmap as you describe. >> >> That has nothing to do with mmap() semantics. You mmap() writeable memory >> and then you change the permissions via mprotect(). mprotect() calls into >> LSM and depending on policy and security model this will reject the >> request. >> >> Andy was pointing out that the SGX ioctl bypasses the LSM mechanics which >> is obviously a bad thing. > > We could modify the driver such that when you call ioctl EADD, the page > table permissions need to be the PAGEINFO.SECINFO.FLAGS | PROT_WRITE, > otherwise you get EPERM or so. After EADD, if you want, you can restrict Actually, I don't think you even need to include PAGEINFO.SECINFO.FLAGS, you just need to ensure PROT_WRITE. Regular page table checks take care of PAGEINFO.SECINFO.FLAGS. -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 13:50, Thomas Gleixner wrote: > On Fri, 19 Apr 2019, Jethro Beekman wrote: >> On 2019-04-19 13:39, Thomas Gleixner wrote: >>> On Fri, 19 Apr 2019, Jethro Beekman wrote: >>> >>>> On 2019-04-19 08:27, Andy Lutomirski wrote: >>>>> There are many, >>>>> many Linux systems that enforce a policy that *all* executable text >>>>> needs to come from a verified source. On these systems, you can't >>>>> mmap some writable memory, write to it, and then change it to >>>>> executable. >>>> >>>> How is this implemented on those systems? AFAIK there's no kernel config >>>> option that changes the semantics of mmap as you describe. >>> >>> That has nothing to do with mmap() semantics. You mmap() writeable memory >>> and then you change the permissions via mprotect(). mprotect() calls into >>> LSM and depending on policy and security model this will reject the >>> request. >>> >>> Andy was pointing out that the SGX ioctl bypasses the LSM mechanics which >>> is obviously a bad thing. >> >> We could modify the driver such that when you call ioctl EADD, the page >> table permissions need to be the PAGEINFO.SECINFO.FLAGS | PROT_WRITE, >> otherwise you get EPERM or so. After EADD, if you want, you can restrict >> the page table permissions again using mprotect but the page table >> permissions don't really matter for SGX. > > And the point of that is? That you still can cirumvent LSM for feeding > executable code into SGX. How? LSM would see that you're trying to map a page RWX so you can do your ioctl? > No, we are not making special cases and exceptions for SGX. Maybe I didn't express myself clearly? I don't think I was suggesting anything like that. -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 13:39, Thomas Gleixner wrote: > On Fri, 19 Apr 2019, Jethro Beekman wrote: > >> On 2019-04-19 08:27, Andy Lutomirski wrote: >>> There are many, >>> many Linux systems that enforce a policy that *all* executable text >>> needs to come from a verified source. On these systems, you can't >>> mmap some writable memory, write to it, and then change it to >>> executable. >> >> How is this implemented on those systems? AFAIK there's no kernel config >> option that changes the semantics of mmap as you describe. > > That has nothing to do with mmap() semantics. You mmap() writeable memory > and then you change the permissions via mprotect(). mprotect() calls into > LSM and depending on policy and security model this will reject the > request. > > Andy was pointing out that the SGX ioctl bypasses the LSM mechanics which > is obviously a bad thing. We could modify the driver such that when you call ioctl EADD, the page table permissions need to be the PAGEINFO.SECINFO.FLAGS | PROT_WRITE, otherwise you get EPERM or so. After EADD, if you want, you can restrict the page table permissions again using mprotect but the page table permissions don't really matter for SGX. -- Jethro Beekman | Fortanix
Re: [PATCH v20 00/28] Intel SGX1 support
On 2019-04-19 08:27, Andy Lutomirski wrote: > There are many, > many Linux systems that enforce a policy that *all* executable text > needs to come from a verified source. On these systems, you can't > mmap some writable memory, write to it, and then change it to > executable. How is this implemented on those systems? AFAIK there's no kernel config option that changes the semantics of mmap as you describe. -- Jethro Beekman | Fortanix
Re: [PATCH v19,RESEND 24/27] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2019-03-20 11:30, Xing, Cedric wrote: +/** + * __vdso_sgx_enter_enclave() - Enter an SGX enclave + * + * %eax:ENCLU leaf, must be EENTER or ERESUME + * %rbx:TCS, must be non-NULL + * %rcx:Optional pointer to 'struct sgx_enclave_exception' + * + * Return: + * 0 on a clean entry/exit to/from the enclave + * -EINVAL if ENCLU leaf is not allowed or if TCS is NULL + * -EFAULT if ENCLU or the enclave faults + * + * Note that __vdso_sgx_enter_enclave() is not compliant with the x86- 64 ABI. + * All registers except RSP must be treated as volatile from the +caller's + * perspective, including but not limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc... + * Conversely, the enclave being run must preserve the untrusted RSP and stack. By requiring preservation of RSP at both AEX and EEXIT, this precludes the possibility of using the untrusted stack as temporary storage by enclaves. While that looks reasonable at first glance, I'm afraid it isn't the case in reality. The untrusted stack is inarguably the most convenient way for data exchange between an enclave and its enclosing process, and is in fact being used for that purpose by almost all existing enclaves to date. Given the expectation that this API will be used by all future SGX application, it looks unwise to ban the most convenient and commonly used approach for data exchange. For reference, here's the code in the Intel toolchain responsible for this: https://github.com/intel/linux-sgx/blob/6a0b5ac71f8d16f04e0376f3b2168e80c773dd23/sdk/trts/trts.cpp#L125-L140 Regarding "almost all existing enclaves to date", enclaves built with the Fortanix toolchain don't touch the untrusted stack. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: x86/sgx: uapi change proposal
On 2018-12-19 14:41, Jarkko Sakkinen wrote:
On Wed, Dec 19, 2018 at 08:41:12AM +, Jethro Beekman wrote:
One weird thing is the departure from the normal mmap behavior that the
memory mapping persists even if the original fd is closed. (See man mmap:
"closing the file descriptor does not unmap the region.")
The mmapped region and enclave would be completely disjoint to start
with. The enclave driver code would assume that an enclave VMA exists
when it maps enclave address space to a process.
I.e. VMA would no longer reference to the enclave or vice versa but
you would still create an enclave VMA with mmap().
This is IMHO very clear and well-defined semantics.
struct sgx_enclave_add_page {
__u64 enclave_fd;
__u64 src;
__u64 secinfo;
__u16 mrmask;
} __attribute__((__packed__));
Wouldn't you just pass enclave_fd as the ioctl fd parameter?
I'm still planning to keep the API in the device fd and use enclave_fd
as handle to the enclave address space. I don't see any obvious reason
to change that behavior.
And if we ever add any "global" ioctls, then we would have to define
APIs to both fd's, which would become a mess.
How to specify the address of the page that is being added?
Yes, that is correct and my bad to remove it (just quickly drafted what
I had in mind).
So your plan is that to call EADD, userspace has to pass the device fd
AND the enclave fd AND the enclave address? That seems a little superfluous.
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: x86/sgx: uapi change proposal
On 2018-12-19 13:28, Jarkko Sakkinen wrote:
I have pretty much figured out how to change the driver implementation
from VMA based to file based. Most of the code in the driver can be
reused with not that enormous changes. I think it is a clue that the
architecture is somewhat right because changing the driver this
radically does not seem to require any changes to the core.
One weird thing is the departure from the normal mmap behavior that the
memory mapping persists even if the original fd is closed. (See man
mmap: "closing the file descriptor does not unmap the region.")
Using anon inode is the right choice because it is more robust interface
to be able to create multiple enclaves.
The only remaining open that I have when it comes to implementing this
is the backing storage. From API perspective the most robust choice
would be to revert to use shmem file. It would be easy then to create a
complete construction flow without any dependencies to mm_struct.
I do recognize the issue with accounting but to which process the
backing storage should be accounted anyway in this new paradigm.
I've attached the new uapi header to this email that I'm going forward
with.
/Jarkko
sgx.h
/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
/**
* Copyright(c) 2016-18 Intel Corporation.
*/
#ifndef _UAPI_ASM_X86_SGX_H
#define _UAPI_ASM_X86_SGX_H
#include
#include
#define SGX_MAGIC 0xA4
#define SGX_IOC_ENCLAVE_CREATE \
_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
#define SGX_IOC_ENCLAVE_ADD_PAGE \
_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
#define SGX_IOC_ENCLAVE_INIT \
_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
/* IOCTL return values */
#define SGX_POWER_LOST_ENCLAVE 0x4000
/**
* struct sgx_enclave_create - parameter structure for the
* %SGX_IOC_ENCLAVE_CREATE ioctl
* @src:address for the SECS page data
* @enclave_fd: file handle to the enclave address space (out)
*/
struct sgx_enclave_create {
__u64 src;
__u64 enclave_fd;
};
/**
* struct sgx_enclave_add_page - parameter structure for the
* %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
* @eclave_fd: file handle to the enclave address space
* @src:address for the page data
* @secinfo:address for the SECINFO data
* @mrmask: bitmask for the measured 256 byte chunks
*/
struct sgx_enclave_add_page {
__u64 enclave_fd;
__u64 src;
__u64 secinfo;
__u16 mrmask;
} __attribute__((__packed__));
Wouldn't you just pass enclave_fd as the ioctl fd parameter?
How to specify the address of the page that is being added?
/**
* struct sgx_enclave_init - parameter structure for the
* %SGX_IOC_ENCLAVE_INIT ioctl
* @eclave_fd: file handle to the enclave address space
* @sigstruct: address for the SIGSTRUCT data
*/
struct sgx_enclave_init {
__u64 enclave_fd;
__u64 sigstruct;
}; >
/**
* struct sgx_enclave_set_attribute - parameter structure for the
*%SGX_IOC_ENCLAVE_INIT ioctl
* @addr: address within the ELRANGE
Stray parameter in docs
* @eclave_fd: file handle to the enclave address space
* @attribute_fd: file handle of the attribute file in the securityfs
*/
struct sgx_enclave_set_attribute {
__u64 enclave_fd;
__u64 attribute_fd;
};
What is this for?
#endif /* _UAPI_ASM_X86_SGX_H */
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v4 5/5] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-14 03:01, Sean Christopherson wrote:
+struct sgx_enclave_regs {
+ __u64 rdi;
+ __u64 rsi;
+ __u64 rdx;
+ __u64 r8;
+ __u64 r9;
+ __u64 r10;
+};
This is fine, but why not just cover all 13 normal registers that are
not used by SGX?
Minor comments below.
+/**
+ * struct sgx_enclave_exception - structure to pass register in/out of enclave
Typo in struct name.
+ * by way of __vdso_sgx_enter_enclave
+ *
+ * @rdi: value of %rdi, loaded/saved on enter/exit
+ * @rsi: value of %rsi, loaded/saved on enter/exit
+ * @rdx: value of %rdx, loaded/saved on enter/exit
+ * @r8:value of %r8, loaded/saved on enter/exit
+ * @r9:value of %r9, loaded/saved on enter/exit
+ * @r10: value of %r10, loaded/saved on enter/exit
+ */
+ /* load leaf, TCS and AEP for ENCLU */
+ mov %edi, %eax
+ mov %rsi, %rbx
+ lea 1f(%rip), %rcx
If you move this below the jump, you can use %rcx for @regs
+
+ /* optionally copy @regs to registers */
+ test%rdx, %rdx
+ je 1f
+
+ mov %rdx, %r11
+ mov RDI(%r11), %rdi
+ mov RSI(%r11), %rsi
+ mov RDX(%r11), %rdx
+ mov R8(%r11), %r8
+ mov R9(%r11), %r9
+ mov R10(%r11), %r10
+
+1: enclu
+
+ /* ret = 0 */
+ xor %eax, %eax
+
+ /* optionally copy registers to @regs */
+ mov -0x8(%rsp), %r11
+ test%r11, %r11
+ je 2f
+
+ mov %rdi, RDI(%r11)
+ mov %rsi, RSI(%r11)
+ mov %rdx, RDX(%r11)
+ mov %r8, R8(%r11)
+ mov %r9, R9(%r11)
+ mov %r10, R10(%r11)
Here you can use %rax for @regs and clear it at the end.
+2: pop %rbx
+ pop %r12
+ pop %r13
+ pop %r14
+ pop %r15
+ pop %rbp
+ ret
x86-64 ABI requires that you call CLD here (enclave may set it).
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-08 00:14, Dave Hansen wrote:
On 12/7/18 10:15 AM, Jethro Beekman wrote:
This is not sufficient to support the Fortanix SGX ABI calling
convention, which was designed to be mostly compatible with the SysV
64-bit calling convention. The following registers need to be passed in
to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
registers need to be passed out from an enclave to userspace: RDI, RSI,
RDX, R8, R9.
Are you asking nicely to change the new Linux ABI to be consistent with
your existing ABI? Or, are you saying that the new ABI *must* be
compatible with this previous out-of-tree implementation?
What's being discussed here is one of the alternatives for SGX fault
handling, meant to improve the current status quo of having to use a
signal handler.
I'm merely providing a data point that the currently proposed solution
is not sufficient to support current use of the (ring 3) ENCLU
instruction. You might find this useful in determining whether proposed
kernel features will actually be used by users, and in further
developing this solution or other solutions to the fault handling issue.
If going with the vDSO solution, I think something with semantics closer
to the actual instruction would be preferred, like the following:
notrace __attribute__((naked)) long __vdso_sgx_enclu_with_aep()
{
asm volatile(
" lea 2f(%%rip), %%rcx\n"
"1:enclu\n"
"2: ret\n"
".pushsection .fixup, \"ax\" \n"
"3:jmp 2b\n"
".popsection\n"
_ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
:::
);
}
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-08 00:14, Dave Hansen wrote:
On 12/7/18 10:15 AM, Jethro Beekman wrote:
This is not sufficient to support the Fortanix SGX ABI calling
convention, which was designed to be mostly compatible with the SysV
64-bit calling convention. The following registers need to be passed in
to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following
registers need to be passed out from an enclave to userspace: RDI, RSI,
RDX, R8, R9.
Are you asking nicely to change the new Linux ABI to be consistent with
your existing ABI? Or, are you saying that the new ABI *must* be
compatible with this previous out-of-tree implementation?
What's being discussed here is one of the alternatives for SGX fault
handling, meant to improve the current status quo of having to use a
signal handler.
I'm merely providing a data point that the currently proposed solution
is not sufficient to support current use of the (ring 3) ENCLU
instruction. You might find this useful in determining whether proposed
kernel features will actually be used by users, and in further
developing this solution or other solutions to the fault handling issue.
If going with the vDSO solution, I think something with semantics closer
to the actual instruction would be preferred, like the following:
notrace __attribute__((naked)) long __vdso_sgx_enclu_with_aep()
{
asm volatile(
" lea 2f(%%rip), %%rcx\n"
"1:enclu\n"
"2: ret\n"
".pushsection .fixup, \"ax\" \n"
"3:jmp 2b\n"
".popsection\n"
_ASM_VDSO_EXTABLE_HANDLE(1b, 3b)
:::
);
}
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-07 22:01, Dr. Greg wrote: Baidu and Fortanix are working on Trusted RunTime Systems (TRTS) based on RUST, I believe, so this will affect them to the extent that they are implementing their own low level enclave runtime support or they may be simply building on top of the low level Intel TRTS. Perhaps Jethro would comment on these issues if he could. As far as I know, Baidu merely provides Rust bindings to the Intel SDK. As far as our requirements, I've sent those in my previous email. I'm assuming that in the proposed model the URTS would interrogate the VDSO to determine the availability of entry and exception handling support and then setup the appropriate infrastructure and exit handler? VDSO's are typically the domain of the system library. Given the nature of SGX I couldn't even conceive of Glibc offering support and, if it was acceptable to provide support, the potential timeframe that would be involved in seeing deployment in the field. As a result, do you anticipate the need for a 'flag day' with respect to URTS/PSW/SDK support for all of this? It is my understanding that the use of the vDSO enclave entry will be optional. i.e., if your application/library/enclave combination installs a signal handler and calls ENCLU directly, that would still work. Of course, using the vDSO will be very strongly recommended. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-07 22:01, Dr. Greg wrote: Baidu and Fortanix are working on Trusted RunTime Systems (TRTS) based on RUST, I believe, so this will affect them to the extent that they are implementing their own low level enclave runtime support or they may be simply building on top of the low level Intel TRTS. Perhaps Jethro would comment on these issues if he could. As far as I know, Baidu merely provides Rust bindings to the Intel SDK. As far as our requirements, I've sent those in my previous email. I'm assuming that in the proposed model the URTS would interrogate the VDSO to determine the availability of entry and exception handling support and then setup the appropriate infrastructure and exit handler? VDSO's are typically the domain of the system library. Given the nature of SGX I couldn't even conceive of Glibc offering support and, if it was acceptable to provide support, the potential timeframe that would be involved in seeing deployment in the field. As a result, do you anticipate the need for a 'flag day' with respect to URTS/PSW/SDK support for all of this? It is my understanding that the use of the vDSO enclave entry will be optional. i.e., if your application/library/enclave combination installs a signal handler and calls ENCLU directly, that would still work. Of course, using the vDSO will be very strongly recommended. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-07 03:49, Sean Christopherson wrote: ... diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.c b/arch/x86/entry/vdso/vsgx_enter_enclave.c new file mode 100644 index ..896c2eb079bb --- /dev/null +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.c ... +enter_enclave: + asm volatile( + /* +* When an event occurs in an enclave, hardware first exits the +* enclave to the AEP, switching CPU context along the way, and +* *then* delivers the event as usual. As part of the context +* switching, registers are loaded with synthetic state (except +* BP and SP, which are saved/restored). The defined synthetic +* state loads registers so that simply executing ENCLU will do +* ERESUME, e.g. RAX=4, RBX=TCS and RCX=AEP after an AEE. So, +* we only need to load RAX, RBX and RCX for the initial entry. +* The AEP can point at that same ENCLU, fixup will jump us out +* if an exception was unhandled. +*/ + " lea 1f(%%rip), %%rcx\n" + "1:enclu\n" + "2:\n" + + ".pushsection .fixup, \"ax\" \n" + "3:jmp 2b\n" + ".popsection\n" + _ASM_VDSO_EXTABLE_HANDLE(1b, 3b) + + : "=a"(leaf), "=D" (rdi), "=S" (rsi), "=d" (rdx) + : "a" (leaf), "b" (tcs), "D" (priv) + : "cc", "memory", + "rcx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15" + ); This is not sufficient to support the Fortanix SGX ABI calling convention, which was designed to be mostly compatible with the SysV 64-bit calling convention. The following registers need to be passed in to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following registers need to be passed out from an enclave to userspace: RDI, RSI, RDX, R8, R9. You can find the ABI specification at https://github.com/fortanix/rust-sgx/blob/master/doc/FORTANIX-SGX-ABI.md#enclave-calling-convention -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 4/4] x86/vdso: Add __vdso_sgx_enter_enclave() to wrap SGX enclave transitions
On 2018-12-07 03:49, Sean Christopherson wrote: ... diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.c b/arch/x86/entry/vdso/vsgx_enter_enclave.c new file mode 100644 index ..896c2eb079bb --- /dev/null +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.c ... +enter_enclave: + asm volatile( + /* +* When an event occurs in an enclave, hardware first exits the +* enclave to the AEP, switching CPU context along the way, and +* *then* delivers the event as usual. As part of the context +* switching, registers are loaded with synthetic state (except +* BP and SP, which are saved/restored). The defined synthetic +* state loads registers so that simply executing ENCLU will do +* ERESUME, e.g. RAX=4, RBX=TCS and RCX=AEP after an AEE. So, +* we only need to load RAX, RBX and RCX for the initial entry. +* The AEP can point at that same ENCLU, fixup will jump us out +* if an exception was unhandled. +*/ + " lea 1f(%%rip), %%rcx\n" + "1:enclu\n" + "2:\n" + + ".pushsection .fixup, \"ax\" \n" + "3:jmp 2b\n" + ".popsection\n" + _ASM_VDSO_EXTABLE_HANDLE(1b, 3b) + + : "=a"(leaf), "=D" (rdi), "=S" (rsi), "=d" (rdx) + : "a" (leaf), "b" (tcs), "D" (priv) + : "cc", "memory", + "rcx", "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15" + ); This is not sufficient to support the Fortanix SGX ABI calling convention, which was designed to be mostly compatible with the SysV 64-bit calling convention. The following registers need to be passed in to an enclave from userspace: RDI, RSI, RDX, R8, R9, R10. The following registers need to be passed out from an enclave to userspace: RDI, RSI, RDX, R8, R9. You can find the ABI specification at https://github.com/fortanix/rust-sgx/blob/master/doc/FORTANIX-SGX-ABI.md#enclave-calling-convention -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-21 04:25, Jarkko Sakkinen wrote: On Tue, Nov 20, 2018 at 07:19:37AM -0800, Andy Lutomirski wrote: general by mucking with some regs and retrying -- that will infinite loop and confuse everyone. I'm not even 100% convinced that decoding the insn stream is useful -- AEP can point to something that isn't ENCLU. In my return-to-AEP approach to whole point was not to do any decoding but instead have something else always in the AEP handler than just ENCLU. No instruction decoding. No RIP manipulation. IOW the kernel needs to know *when* to apply this special behavior. Sadly there is no bit in the exception frame that says "came from SGX". Jarkko, can you please explain you solution in detail? The CPU receives an exception. This will be handled by the kernel exception handler. What information does the kernel exception handler use to determine whether to deliver the exception as a regular signal to the process, or whether to set the special registers values for userspace and just continue executing the process manually? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-21 04:25, Jarkko Sakkinen wrote: On Tue, Nov 20, 2018 at 07:19:37AM -0800, Andy Lutomirski wrote: general by mucking with some regs and retrying -- that will infinite loop and confuse everyone. I'm not even 100% convinced that decoding the insn stream is useful -- AEP can point to something that isn't ENCLU. In my return-to-AEP approach to whole point was not to do any decoding but instead have something else always in the AEP handler than just ENCLU. No instruction decoding. No RIP manipulation. IOW the kernel needs to know *when* to apply this special behavior. Sadly there is no bit in the exception frame that says "came from SGX". Jarkko, can you please explain you solution in detail? The CPU receives an exception. This will be handled by the kernel exception handler. What information does the kernel exception handler use to determine whether to deliver the exception as a regular signal to the process, or whether to set the special registers values for userspace and just continue executing the process manually? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v17 18/23] platform/x86: Intel SGX driver
On 2018-11-19 20:36, Jarkko Sakkinen wrote: Question: should be dissolve the driver completely and move this code to arch/x86/kernel/cpu/sgx/ (and rename intel_sgx.c as main.c)? Swapping patch removes the possibility to compile this as a module anyway. No. We should keep the capability to build this as a module for other users of SGX. What is the swapping patch and why doesn't allow building as a module? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v17 18/23] platform/x86: Intel SGX driver
On 2018-11-19 20:36, Jarkko Sakkinen wrote: Question: should be dissolve the driver completely and move this code to arch/x86/kernel/cpu/sgx/ (and rename intel_sgx.c as main.c)? Swapping patch removes the possibility to compile this as a module anyway. No. We should keep the capability to build this as a module for other users of SGX. What is the swapping patch and why doesn't allow building as a module? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-18 18:32, Jarkko Sakkinen wrote: On Sun, Nov 18, 2018 at 09:15:48AM +0200, Jarkko Sakkinen wrote: On Thu, Nov 01, 2018 at 10:53:40AM -0700, Andy Lutomirski wrote: Hi all- The people working on SGX enablement are grappling with a somewhat annoying issue: the x86 EENTER instruction is used from user code and can, as part of its normal-ish operation, raise an exception. It is also highly likely to be used from a library, and signal handling in libraries is unpleasant at best. There's been some discussion of adding a vDSO entry point to wrap EENTER and do something sensible with the exceptions, but I'm wondering if a more general mechanism would be helpful. I haven't really followed all of this discussion because I've been busy working on the patch set but for me all of these approaches look awfully complicated. I'll throw my own suggestion and apologize if this has been already suggested and discarded: return-to-AEP. My idea is to do just a small extension to SGX AEX handling. At the moment hardware will RAX, RBX and RCX with ERESUME parameters. We can fill extend this by filling other three spare registers with exception information. AEP handler can then do whatever it wants to do with this information or just do ERESUME. A correction here. In practice this will add a requirement to have a bit more complicated AEP code (check the regs for exceptions) than before and not just bytes for ENCLU. e.g. AEP handler should be along the lines 1. #PF (or #UD or) happens. Kernel fills the registers when it cannot handle the exception and returns back to user space i.e. to the AEP handler. 2. Check the registers containing exception information. If they have been filled, take whatever actions user space wants to take. 3. Otherwise, just ERESUME. From my point of view this is making the AEP parameter useful. Its standard use is just weird (always point to a place just containing ENCLU bytes, why the heck it even exists). I like this solution. Keeps things simple. One question: when an exception occurs, how does the kernel know whether to set special registers or send a signal? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-18 18:32, Jarkko Sakkinen wrote: On Sun, Nov 18, 2018 at 09:15:48AM +0200, Jarkko Sakkinen wrote: On Thu, Nov 01, 2018 at 10:53:40AM -0700, Andy Lutomirski wrote: Hi all- The people working on SGX enablement are grappling with a somewhat annoying issue: the x86 EENTER instruction is used from user code and can, as part of its normal-ish operation, raise an exception. It is also highly likely to be used from a library, and signal handling in libraries is unpleasant at best. There's been some discussion of adding a vDSO entry point to wrap EENTER and do something sensible with the exceptions, but I'm wondering if a more general mechanism would be helpful. I haven't really followed all of this discussion because I've been busy working on the patch set but for me all of these approaches look awfully complicated. I'll throw my own suggestion and apologize if this has been already suggested and discarded: return-to-AEP. My idea is to do just a small extension to SGX AEX handling. At the moment hardware will RAX, RBX and RCX with ERESUME parameters. We can fill extend this by filling other three spare registers with exception information. AEP handler can then do whatever it wants to do with this information or just do ERESUME. A correction here. In practice this will add a requirement to have a bit more complicated AEP code (check the regs for exceptions) than before and not just bytes for ENCLU. e.g. AEP handler should be along the lines 1. #PF (or #UD or) happens. Kernel fills the registers when it cannot handle the exception and returns back to user space i.e. to the AEP handler. 2. Check the registers containing exception information. If they have been filled, take whatever actions user space wants to take. 3. Otherwise, just ERESUME. From my point of view this is making the AEP parameter useful. Its standard use is just weird (always point to a place just containing ENCLU bytes, why the heck it even exists). I like this solution. Keeps things simple. One question: when an exception occurs, how does the kernel know whether to set special registers or send a signal? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-07 02:17, Andy Lutomirski wrote: On Tue, Nov 6, 2018 at 4:02 PM Sean Christopherson wrote: /* * EEXIT or EENTER faulted. In the latter case, %RAX already holds some * fault indicator, e.g. -EFAULT. */ eexit_or_eenter_fault: ret But userspace wants to know whether it was a fault or not. So I think we either need two landing pads or we need to hijack a flag bit (are there any known-zeroed flag bits after EEXIT?) to say whether it was a fault. And, if it was a fault, we should give the vector, the sanitized error code, and possibly CR2. On AEX, %rax will contain ENCLU_LEAF_ERESUME (0x3). On EEXIT, %rax will contain ENCLU_LEAF_EEXIT (0x4). -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-07 02:17, Andy Lutomirski wrote: On Tue, Nov 6, 2018 at 4:02 PM Sean Christopherson wrote: /* * EEXIT or EENTER faulted. In the latter case, %RAX already holds some * fault indicator, e.g. -EFAULT. */ eexit_or_eenter_fault: ret But userspace wants to know whether it was a fault or not. So I think we either need two landing pads or we need to hijack a flag bit (are there any known-zeroed flag bits after EEXIT?) to say whether it was a fault. And, if it was a fault, we should give the vector, the sanitized error code, and possibly CR2. On AEX, %rax will contain ENCLU_LEAF_ERESUME (0x3). On EEXIT, %rax will contain ENCLU_LEAF_EEXIT (0x4). -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v15 16/23] x86/sgx: Enumerate and track EPC sections
On 2018-11-02 16:11, Jarkko Sakkinen wrote:
diff --git a/arch/x86/kernel/cpu/intel_sgx.c b/arch/x86/kernel/cpu/intel_sgx.c
new file mode 100644
index ..b86aa4111592
--- /dev/null
+++ b/arch/x86/kernel/cpu/intel_sgx.c
...
+static __init int sgx_page_cache_init(void)
+{
+ u32 eax, ebx, ecx, edx, type;
+ u64 pa, size;
+ int ret;
+ int i;
+
+ BUILD_BUG_ON(SGX_MAX_EPC_SECTIONS > (SGX_EPC_SECTION_MASK + 1));
+
+ for (i = 0; i < SGX_MAX_EPC_SECTIONS; i++) {
Perhaps print a warning if there are more than SGX_MAX_EPC_SECTIONS
sections reported by CPUID.
+ cpuid_count(SGX_CPUID, i + SGX_CPUID_FIRST_VARIABLE_SUB_LEAF,
+ , , , );
+
+ type = eax & SGX_CPUID_SUB_LEAF_TYPE_MASK;
+ if (type == SGX_CPUID_SUB_LEAF_INVALID)
+ break;
+ if (type != SGX_CPUID_SUB_LEAF_EPC_SECTION) {
+ pr_err_once("sgx: Unknown sub-leaf type: %u\n", type);
+ continue;
+ }
+
+ pa = sgx_calc_section_metric(eax, ebx);
+ size = sgx_calc_section_metric(ecx, edx);
+ pr_info("sgx: EPC section 0x%llx-0x%llx\n", pa, pa + size - 1);
+
+ ret = sgx_init_epc_section(pa, size, i, _epc_sections[i]);
+ if (ret) {
+ sgx_page_cache_teardown();
+ return ret;
+ }
+
+ sgx_nr_epc_sections++;
+ }
+
+ if (!sgx_nr_epc_sections) {
+ pr_err("sgx: There are zero EPC sections.\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [PATCH v15 16/23] x86/sgx: Enumerate and track EPC sections
On 2018-11-02 16:11, Jarkko Sakkinen wrote:
diff --git a/arch/x86/kernel/cpu/intel_sgx.c b/arch/x86/kernel/cpu/intel_sgx.c
new file mode 100644
index ..b86aa4111592
--- /dev/null
+++ b/arch/x86/kernel/cpu/intel_sgx.c
...
+static __init int sgx_page_cache_init(void)
+{
+ u32 eax, ebx, ecx, edx, type;
+ u64 pa, size;
+ int ret;
+ int i;
+
+ BUILD_BUG_ON(SGX_MAX_EPC_SECTIONS > (SGX_EPC_SECTION_MASK + 1));
+
+ for (i = 0; i < SGX_MAX_EPC_SECTIONS; i++) {
Perhaps print a warning if there are more than SGX_MAX_EPC_SECTIONS
sections reported by CPUID.
+ cpuid_count(SGX_CPUID, i + SGX_CPUID_FIRST_VARIABLE_SUB_LEAF,
+ , , , );
+
+ type = eax & SGX_CPUID_SUB_LEAF_TYPE_MASK;
+ if (type == SGX_CPUID_SUB_LEAF_INVALID)
+ break;
+ if (type != SGX_CPUID_SUB_LEAF_EPC_SECTION) {
+ pr_err_once("sgx: Unknown sub-leaf type: %u\n", type);
+ continue;
+ }
+
+ pa = sgx_calc_section_metric(eax, ebx);
+ size = sgx_calc_section_metric(ecx, edx);
+ pr_info("sgx: EPC section 0x%llx-0x%llx\n", pa, pa + size - 1);
+
+ ret = sgx_init_epc_section(pa, size, i, _epc_sections[i]);
+ if (ret) {
+ sgx_page_cache_teardown();
+ return ret;
+ }
+
+ sgx_nr_epc_sections++;
+ }
+
+ if (!sgx_nr_epc_sections) {
+ pr_err("sgx: There are zero EPC sections.\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
--
Jethro Beekman | Fortanix
smime.p7s
Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 10:01, Andy Lutomirski wrote: On Fri, Nov 2, 2018 at 9:56 AM Jethro Beekman wrote: On 2018-11-02 09:52, Sean Christopherson wrote: On Fri, Nov 02, 2018 at 04:37:10PM +, Jethro Beekman wrote: On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. To what end? Userspace gets no indication as to why the AEX occurred. And if exceptions are getting transfered to userspace the trampoline would effectively be handling only INTR, NMI, #MC and EPC #PF. Various reasons... Userspace may have established an exception handling convention with the enclave (by setting TCS.NSSA > 1) and may want to call EENTER instead of ERESUME. Ugh, I sincerely hope that a future ISA extension lets the kernel return directly back to enclave mode so that AEX events become entirely invisible to user code. Can you explain how this would work for things like #BR/#DE/#UD that need to be fixed up by code running in the enclave before it can be resumed? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 10:01, Andy Lutomirski wrote: On Fri, Nov 2, 2018 at 9:56 AM Jethro Beekman wrote: On 2018-11-02 09:52, Sean Christopherson wrote: On Fri, Nov 02, 2018 at 04:37:10PM +, Jethro Beekman wrote: On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. To what end? Userspace gets no indication as to why the AEX occurred. And if exceptions are getting transfered to userspace the trampoline would effectively be handling only INTR, NMI, #MC and EPC #PF. Various reasons... Userspace may have established an exception handling convention with the enclave (by setting TCS.NSSA > 1) and may want to call EENTER instead of ERESUME. Ugh, I sincerely hope that a future ISA extension lets the kernel return directly back to enclave mode so that AEX events become entirely invisible to user code. Can you explain how this would work for things like #BR/#DE/#UD that need to be fixed up by code running in the enclave before it can be resumed? -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 09:52, Sean Christopherson wrote: On Fri, Nov 02, 2018 at 04:37:10PM +, Jethro Beekman wrote: On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. To what end? Userspace gets no indication as to why the AEX occurred. And if exceptions are getting transfered to userspace the trampoline would effectively be handling only INTR, NMI, #MC and EPC #PF. Various reasons... Userspace may have established an exception handling convention with the enclave (by setting TCS.NSSA > 1) and may want to call EENTER instead of ERESUME. Userspace may want fine-grained control over enclave scheduling (e.g. SGX-Step) -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 09:52, Sean Christopherson wrote: On Fri, Nov 02, 2018 at 04:37:10PM +, Jethro Beekman wrote: On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. To what end? Userspace gets no indication as to why the AEX occurred. And if exceptions are getting transfered to userspace the trampoline would effectively be handling only INTR, NMI, #MC and EPC #PF. Various reasons... Userspace may have established an exception handling convention with the enclave (by setting TCS.NSSA > 1) and may want to call EENTER instead of ERESUME. Userspace may want fine-grained control over enclave scheduling (e.g. SGX-Step) -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: RFC: userspace exception fixups
On 2018-11-02 09:30, Sean Christopherson wrote: ... The intended convention for EENTER is to have an ENCLU at the AEX target ... ... to further enforce that the AEX target needs to be ENCLU. Some SGX runtimes may want to use a different AEX target. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v14 09/19] x86/mm: x86/sgx: Signal SEGV_SGXERR for #PFs w/ PF_SGX
On 2018-09-27 06:56, Jarkko Sakkinen wrote: On Wed, Sep 26, 2018 at 02:45:17PM -0700, Dave Hansen wrote: On 09/26/2018 02:15 PM, Andy Lutomirski wrote: Could we perhaps have a little vDSO entry (or syscall, I suppose) that runs an enclave an returns an error code, and rig up the #PF handler to check if the error happened in the vDSO entry and fix it up rather than sending a signal? For me this plan sounds simple and sound. I support avoiding the need for a signal handler for various SGX-specific operations. Looking forward to v15. I have some thoughts regarding the design of the vDSO function. Please consider the following as you work on the next patch set. 1) Even though the vDSO function exists, userspace may still call `ENCLU[EENTER]` manually, so the fault handling as described in the current patch should also be maintained. 2) All the information that would normally be provided through the signal handler (x86 fault number, reason) should be provided to userspace. 3) vDSO functions should be provided for all standard non-enclave ENCLU leafs, and should support most ways that an application might want to use. This includes: * EENTER with a automatic AEX handler (as in Jarkko's sgx_get_token example) * EENTER & ERESUME with a user-specified AEX handler, or possibly just a special return value from the ENCLU function on AEX 4) I think the vDSO functions should have a special calling convention (not conforming to the standard SysV ABI), such that most registers are passed between user space and enclave space untouched. Basically, only RAX, RBX, RCX are available as input and output registers. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v14 09/19] x86/mm: x86/sgx: Signal SEGV_SGXERR for #PFs w/ PF_SGX
On 2018-09-27 06:56, Jarkko Sakkinen wrote: On Wed, Sep 26, 2018 at 02:45:17PM -0700, Dave Hansen wrote: On 09/26/2018 02:15 PM, Andy Lutomirski wrote: Could we perhaps have a little vDSO entry (or syscall, I suppose) that runs an enclave an returns an error code, and rig up the #PF handler to check if the error happened in the vDSO entry and fix it up rather than sending a signal? For me this plan sounds simple and sound. I support avoiding the need for a signal handler for various SGX-specific operations. Looking forward to v15. I have some thoughts regarding the design of the vDSO function. Please consider the following as you work on the next patch set. 1) Even though the vDSO function exists, userspace may still call `ENCLU[EENTER]` manually, so the fault handling as described in the current patch should also be maintained. 2) All the information that would normally be provided through the signal handler (x86 fault number, reason) should be provided to userspace. 3) vDSO functions should be provided for all standard non-enclave ENCLU leafs, and should support most ways that an application might want to use. This includes: * EENTER with a automatic AEX handler (as in Jarkko's sgx_get_token example) * EENTER & ERESUME with a user-specified AEX handler, or possibly just a special return value from the ENCLU function on AEX 4) I think the vDSO functions should have a special calling convention (not conforming to the standard SysV ABI), such that most registers are passed between user space and enclave space untouched. Basically, only RAX, RBX, RCX are available as input and output registers. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v12 11/13] platform/x86: Intel SGX driver
On 2018-07-03 11:19, Jarkko Sakkinen wrote: +/* IOCTL return values */ +#define SGX_POWER_LOST_ENCLAVE 0x4000 +#define SGX_LE_ROLLBACK0x4001 I don't think SGX_LE_ROLLBACK is used anymore. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v12 11/13] platform/x86: Intel SGX driver
On 2018-07-03 11:19, Jarkko Sakkinen wrote: +/* IOCTL return values */ +#define SGX_POWER_LOST_ENCLAVE 0x4000 +#define SGX_LE_ROLLBACK0x4001 I don't think SGX_LE_ROLLBACK is used anymore. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On 2018-06-20 11:16, Jethro Beekman wrote: > This last bit is also repeated in different words in Table 35-2 and > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > itself is locked. Meaning the MSRs are either locked with Intel's key > hash, or not locked at all. Actually, this might be a documentation bug. I have some test hardware and I was able to configure the MSRs in the BIOS and then read the MSRs after boot like this: MSR 0x3a 0x00040005 MSR 0x8c 0x20180620 MSR 0x8d 0x20180620 MSR 0x8e 0x20180620 MSR 0x8f 0x20180620 Since this is not production hardware, it could also be a CPU bug of course. If it is indeed possible to configure AND lock the MSR values to non-Intel values, I'm very much in favor of Nathaniels proposal to treat the launch enclave like any other firmware blob. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On 2018-06-20 11:16, Jethro Beekman wrote: > This last bit is also repeated in different words in Table 35-2 and > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > itself is locked. Meaning the MSRs are either locked with Intel's key > hash, or not locked at all. Actually, this might be a documentation bug. I have some test hardware and I was able to configure the MSRs in the BIOS and then read the MSRs after boot like this: MSR 0x3a 0x00040005 MSR 0x8c 0x20180620 MSR 0x8d 0x20180620 MSR 0x8e 0x20180620 MSR 0x8f 0x20180620 Since this is not production hardware, it could also be a CPU bug of course. If it is indeed possible to configure AND lock the MSR values to non-Intel values, I'm very much in favor of Nathaniels proposal to treat the launch enclave like any other firmware blob. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On 2018-06-20 09:28, Nathaniel McCallum wrote: As I understand it, the current policy models under discussion look like this: 1. SGX w/o FLC (not being merged) looks like this: Intel CPU => (Intel signed) launch enclave => enclaves I think you mean: Intel CPU => kernel => (Intel signed) launch enclave => enclaves 2. SGX w/ FLC, looks like this: Intel CPU => kernel => launch enclave => enclaves 3. Andy is proposing this: Intel CPU => kernel => enclaves This proposal is based on the fact that if the kernel can write to the MSRs then a kernel compromise allows an attacker to run their own launch enclave and therefore having an independent launch enclave adds only complexity but not security. Is it possible to restrict the ability of the kernel to change the MSRs? For example, could a UEFI module manage the MSRs? Could the launch enclave live entirely within that UEFI module? On 2017-03-17 09:15, Jethro Beekman wrote: > While collecting my thoughts about the issue I read through the > documentation again and it seems that it will not be possible for a > platform to lock in a non-Intel hash at all. From Section 39.1.4 of the > latest Intel SDM: > > > IA32_SGXLEPUBKEYHASH defaults to digest of Intel’s launch enclave > > signing key after reset. > > > > IA32_FEATURE_CONTROL bit 17 controls the permissions on the > > IA32_SGXLEPUBKEYHASH MSRs when CPUID.(EAX=12H, ECX=00H):EAX[0] = 1. > > If IA32_FEATURE_CONTROL is locked with bit 17 set, > > IA32_SGXLEPUBKEYHASH MSRs are reconfigurable (writeable). If either > > IA32_FEATURE_CONTROL is not locked or bit 17 is clear, the MSRs are > > read only. > > This last bit is also repeated in different words in Table 35-2 and > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > itself is locked. Meaning the MSRs are either locked with Intel's key > hash, or not locked at all. 4. I am suggesting this: Intel CPU => UEFI module => enclaves Under this architecture, the kernel isn't involved in policy at all and users get exactly the same freedoms they already have with Secure Boot. Further, the launch enclave can be independently updated and could be distributed in linux-firmware. The UEFI module can also be shared across operating systems. If I want to have my own enclave policy, then I can build the UEFI module myself, with my modifications, and I can disable Secure Boot. Alternatively, distributions that want to set their own policies can build their own UEFI module and sign it with their vendor key. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On 2018-06-20 09:28, Nathaniel McCallum wrote: As I understand it, the current policy models under discussion look like this: 1. SGX w/o FLC (not being merged) looks like this: Intel CPU => (Intel signed) launch enclave => enclaves I think you mean: Intel CPU => kernel => (Intel signed) launch enclave => enclaves 2. SGX w/ FLC, looks like this: Intel CPU => kernel => launch enclave => enclaves 3. Andy is proposing this: Intel CPU => kernel => enclaves This proposal is based on the fact that if the kernel can write to the MSRs then a kernel compromise allows an attacker to run their own launch enclave and therefore having an independent launch enclave adds only complexity but not security. Is it possible to restrict the ability of the kernel to change the MSRs? For example, could a UEFI module manage the MSRs? Could the launch enclave live entirely within that UEFI module? On 2017-03-17 09:15, Jethro Beekman wrote: > While collecting my thoughts about the issue I read through the > documentation again and it seems that it will not be possible for a > platform to lock in a non-Intel hash at all. From Section 39.1.4 of the > latest Intel SDM: > > > IA32_SGXLEPUBKEYHASH defaults to digest of Intel’s launch enclave > > signing key after reset. > > > > IA32_FEATURE_CONTROL bit 17 controls the permissions on the > > IA32_SGXLEPUBKEYHASH MSRs when CPUID.(EAX=12H, ECX=00H):EAX[0] = 1. > > If IA32_FEATURE_CONTROL is locked with bit 17 set, > > IA32_SGXLEPUBKEYHASH MSRs are reconfigurable (writeable). If either > > IA32_FEATURE_CONTROL is not locked or bit 17 is clear, the MSRs are > > read only. > > This last bit is also repeated in different words in Table 35-2 and > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > itself is locked. Meaning the MSRs are either locked with Intel's key > hash, or not locked at all. 4. I am suggesting this: Intel CPU => UEFI module => enclaves Under this architecture, the kernel isn't involved in policy at all and users get exactly the same freedoms they already have with Secure Boot. Further, the launch enclave can be independently updated and could be distributed in linux-firmware. The UEFI module can also be shared across operating systems. If I want to have my own enclave policy, then I can build the UEFI module myself, with my modifications, and I can disable Secure Boot. Alternatively, distributions that want to set their own policies can build their own UEFI module and sign it with their vendor key. Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v11 09/13] x86, sgx: basic routines for enclave page cache
On 2018-06-19 07:08, Jarkko Sakkinen wrote: On Fri, Jun 08, 2018 at 11:21:48AM -0700, Jethro Beekman wrote: On 2018-06-08 10:09, Jarkko Sakkinen wrote: +/* + * Writing the LE hash MSRs is extraordinarily expensive, e.g. + * 3-4x slower than normal MSRs, so we use a per-cpu cache to + * track the last known value of the MSRs to avoid unnecessarily + * writing the MSRs with the current value. Because most Linux + * kernels will use an LE that is signed with a non-Intel key, I don't think you can predict what most Linux kernels will be doing. I think not initializing the cache to the CPU's initial value is fine, but this particular argument shouldn't appear in the rationale. Are you just referring to the last sentence or the whole paragraph? Just the last sentence. /Jarkko Jethro smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v11 09/13] x86, sgx: basic routines for enclave page cache
On 2018-06-19 07:08, Jarkko Sakkinen wrote: On Fri, Jun 08, 2018 at 11:21:48AM -0700, Jethro Beekman wrote: On 2018-06-08 10:09, Jarkko Sakkinen wrote: +/* + * Writing the LE hash MSRs is extraordinarily expensive, e.g. + * 3-4x slower than normal MSRs, so we use a per-cpu cache to + * track the last known value of the MSRs to avoid unnecessarily + * writing the MSRs with the current value. Because most Linux + * kernels will use an LE that is signed with a non-Intel key, I don't think you can predict what most Linux kernels will be doing. I think not initializing the cache to the CPU's initial value is fine, but this particular argument shouldn't appear in the rationale. Are you just referring to the last sentence or the whole paragraph? Just the last sentence. /Jarkko Jethro smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v11 09/13] x86, sgx: basic routines for enclave page cache
On 2018-06-08 10:09, Jarkko Sakkinen wrote: +/* + * Writing the LE hash MSRs is extraordinarily expensive, e.g. + * 3-4x slower than normal MSRs, so we use a per-cpu cache to + * track the last known value of the MSRs to avoid unnecessarily + * writing the MSRs with the current value. Because most Linux + * kernels will use an LE that is signed with a non-Intel key, I don't think you can predict what most Linux kernels will be doing. I think not initializing the cache to the CPU's initial value is fine, but this particular argument shouldn't appear in the rationale. + * i.e. the first EINIT will need to write the MSRs regardless + * of the cache, the cache is intentionally left uninitialized + * during boot as initializing the cache would be pure overhead + * for the majority of systems. Furthermore, the MSRs are per-cpu + * and the boot-time values aren't guaranteed to be identical + * across cpus, so we'd have to run code all all cpus to properly + * init the cache. All in all, the complexity and overhead of + * initializing the cache is not justified. + */ +static DEFINE_PER_CPU(u64 [4], sgx_le_pubkey_hash_cache); -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v11 09/13] x86, sgx: basic routines for enclave page cache
On 2018-06-08 10:09, Jarkko Sakkinen wrote: +/* + * Writing the LE hash MSRs is extraordinarily expensive, e.g. + * 3-4x slower than normal MSRs, so we use a per-cpu cache to + * track the last known value of the MSRs to avoid unnecessarily + * writing the MSRs with the current value. Because most Linux + * kernels will use an LE that is signed with a non-Intel key, I don't think you can predict what most Linux kernels will be doing. I think not initializing the cache to the CPU's initial value is fine, but this particular argument shouldn't appear in the rationale. + * i.e. the first EINIT will need to write the MSRs regardless + * of the cache, the cache is intentionally left uninitialized + * during boot as initializing the cache would be pure overhead + * for the majority of systems. Furthermore, the MSRs are per-cpu + * and the boot-time values aren't guaranteed to be identical + * across cpus, so we'd have to run code all all cpus to properly + * init the cache. All in all, the complexity and overhead of + * initializing the cache is not justified. + */ +static DEFINE_PER_CPU(u64 [4], sgx_le_pubkey_hash_cache); -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
