From 32ac7f77e7d4ddae7bd7b6707c38f03587e318c9 Mon Sep 17 00:00:00 2001 From: Raoul Strackx Date: Sat, 15 Feb 2025 16:04:29 +0100 Subject: [PATCH 1/3] Generate valid xfrm when required --- intel-sgx/enclave-runner/src/loader.rs | 62 +++++++++++++++++++++++++- intel-sgx/sgxs-loaders/src/isgx/mod.rs | 1 + 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/intel-sgx/enclave-runner/src/loader.rs b/intel-sgx/enclave-runner/src/loader.rs index 6da6079f..b2945102 100644 --- a/intel-sgx/enclave-runner/src/loader.rs +++ b/intel-sgx/enclave-runner/src/loader.rs @@ -4,6 +4,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use std::arch::x86_64::{self, CpuidResult}; use std::fs::File; use std::io::{Error as IoError, ErrorKind, Read, Result as IoResult}; use std::ops::RangeInclusive; @@ -21,6 +22,7 @@ use openssl::{ }; use sgx_isa::{Attributes, AttributesFlags, Miscselect, Sigstruct}; +use sgxs::sgxs::PageReader; use sgxs::crypto::{SgxHashOps, SgxRsaOps}; use sgxs::loader::{Load, MappingInfo, Tcs}; use sgxs::sigstruct::{self, EnclaveHash, Signer}; @@ -163,11 +165,67 @@ impl<'a> EnclaveBuilder<'a> { ret } - fn generate_dummy_signature(&mut self) -> Result { + fn cpuid(eax: u32, ecx: u32) -> Option { + unsafe { + if eax <= x86_64::__get_cpuid_max(0).0 { + Some(x86_64::__cpuid_count(eax, ecx)) + } else { + None + } + } + } + + fn generate_xfrm(max_ssaframesize_in_pages: u32) -> u64 { fn xgetbv0() -> u64 { unsafe { arch::x86_64::_xgetbv(0) } } + debug_assert!(0 < max_ssaframesize_in_pages); + let xcr0 = xgetbv0(); + + // See allorithm of Intel dev manual Chpt 40.7.2.2 + let xfrm = (0..64) + .map(|bit| { + let select = 0x1 << bit; + match bit { + 0 | 1 => select, // Bit 0 and 1 always need to be set + _ => { + if xcr0 & select == 0 { + return 0; + } + + let (base, size) = { + let cpuid = Self::cpuid(0x0d, bit); + let base = cpuid.map_or(0, |c| c.ebx); + let size = cpuid.map_or(0, |c| c.eax); + (base, size) + }; + if max_ssaframesize_in_pages * 0x1000 <= base + size { + return 0; + } + + select + } + } + }) + .fold(0, |xfrm, b| xfrm | b); + + // Intel x86 manual Vol 3 Chpt 13.3: + // "Executing the XSETBV instruction causes a general-protection fault (#GP) if ECX = 0 + // and EAX[17] ≠ EAX[18] (TILECFG and TILEDATA must be enabled together). This implies + // that the value of XCR0[18:17] is always either 00b or 11b." + // The enclave entry code executes xrstor, and we may have just cleared bit 18, so we + // need to correct the invariant + if (xfrm & (0x1 << 17)) != (xfrm & (0x1 << 18)) { + xfrm & !(0x3 << 17) + } else { + xfrm + } + } + + fn generate_dummy_signature(&mut self) -> Result { + let mut enclave = self.enclave.try_clone().unwrap(); + let create_info = PageReader::new(&mut enclave)?; let mut enclave = self.enclave.try_clone().unwrap(); let hash = match self.hash_enclave.take() { Some(f) => f(&mut enclave)?, @@ -177,7 +235,7 @@ impl<'a> EnclaveBuilder<'a> { let attributes = self.attributes.unwrap_or_else(|| Attributes { flags: AttributesFlags::DEBUG | AttributesFlags::MODE64BIT, - xfrm: xgetbv0(), + xfrm: Self::generate_xfrm(create_info.0.ecreate.ssaframesize), }); signer .attributes_flags(attributes.flags, !0) diff --git a/intel-sgx/sgxs-loaders/src/isgx/mod.rs b/intel-sgx/sgxs-loaders/src/isgx/mod.rs index f0cb4a63..56eb3649 100644 --- a/intel-sgx/sgxs-loaders/src/isgx/mod.rs +++ b/intel-sgx/sgxs-loaders/src/isgx/mod.rs @@ -198,6 +198,7 @@ impl EnclaveLoad for InnerDevice { ..Default::default() }; let createdata = ioctl::CreateData { secs: &secs }; + ioctl_unsafe!( Create, ioctl::create(mapping.device.fd.as_raw_fd(), &createdata) From 61fb9d1051665881881ee90b2737c875b6006e89 Mon Sep 17 00:00:00 2001 From: Raoul Strackx Date: Wed, 19 Feb 2025 15:16:10 +0100 Subject: [PATCH 2/3] Addressed reviewer comments --- intel-sgx/enclave-runner/src/loader.rs | 72 ++++++++++++++------------ intel-sgx/sgxs-loaders/src/isgx/mod.rs | 1 - 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/intel-sgx/enclave-runner/src/loader.rs b/intel-sgx/enclave-runner/src/loader.rs index b2945102..61735836 100644 --- a/intel-sgx/enclave-runner/src/loader.rs +++ b/intel-sgx/enclave-runner/src/loader.rs @@ -165,61 +165,67 @@ impl<'a> EnclaveBuilder<'a> { ret } - fn cpuid(eax: u32, ecx: u32) -> Option { - unsafe { - if eax <= x86_64::__get_cpuid_max(0).0 { - Some(x86_64::__cpuid_count(eax, ecx)) - } else { - None + fn generate_xfrm(max_ssaframesize_in_pages: u32) -> u64 { + fn cpuid(eax: u32, ecx: u32) -> Option { + unsafe { + if eax <= x86_64::__get_cpuid_max(0).0 { + Some(x86_64::__cpuid_count(eax, ecx)) + } else { + None + } } } - } - fn generate_xfrm(max_ssaframesize_in_pages: u32) -> u64 { fn xgetbv0() -> u64 { unsafe { arch::x86_64::_xgetbv(0) } } - debug_assert!(0 < max_ssaframesize_in_pages); + debug_assert_ne!(0, max_ssaframesize_in_pages); let xcr0 = xgetbv0(); - // See allorithm of Intel dev manual Chpt 40.7.2.2 + // See algorithm of Intel x86 dev manual Chpt 40.7.2.2 let xfrm = (0..64) .map(|bit| { let select = 0x1 << bit; - match bit { - 0 | 1 => select, // Bit 0 and 1 always need to be set - _ => { - if xcr0 & select == 0 { - return 0; - } - - let (base, size) = { - let cpuid = Self::cpuid(0x0d, bit); - let base = cpuid.map_or(0, |c| c.ebx); - let size = cpuid.map_or(0, |c| c.eax); - (base, size) - }; - if max_ssaframesize_in_pages * 0x1000 <= base + size { - return 0; - } - - select + + if bit == 0 || bit == 1 { + return select; // Bit 0 and 1 always need to be set + } + + if xcr0 & select == 0 { + return 0; + } + + let (base, size) = { + let cpuid = cpuid(0x0d, bit); + let base = cpuid.map_or(0, |c| c.ebx); + + if base == 0 { + (0, 0) + } else { + let size = cpuid.map_or(0, |c| c.eax); + (base, size) } + }; + + if max_ssaframesize_in_pages * 0x1000 < base + size { + return 0; } + + select }) .fold(0, |xfrm, b| xfrm | b); - // Intel x86 manual Vol 3 Chpt 13.3: + // Intel x86 dev manual Vol 3 Chpt 13.3: // "Executing the XSETBV instruction causes a general-protection fault (#GP) if ECX = 0 // and EAX[17] ≠ EAX[18] (TILECFG and TILEDATA must be enabled together). This implies // that the value of XCR0[18:17] is always either 00b or 11b." // The enclave entry code executes xrstor, and we may have just cleared bit 18, so we // need to correct the invariant - if (xfrm & (0x1 << 17)) != (xfrm & (0x1 << 18)) { - xfrm & !(0x3 << 17) - } else { - xfrm + const XCR0_TILE_BITS: u64 = 0b11 << 17; + match xfrm & XCR0_TILE_BITS { + XCR0_TILE_BITS | 0 => xfrm, + _ => xfrm & !XCR0_TILE_BITS, } } diff --git a/intel-sgx/sgxs-loaders/src/isgx/mod.rs b/intel-sgx/sgxs-loaders/src/isgx/mod.rs index 56eb3649..f0cb4a63 100644 --- a/intel-sgx/sgxs-loaders/src/isgx/mod.rs +++ b/intel-sgx/sgxs-loaders/src/isgx/mod.rs @@ -198,7 +198,6 @@ impl EnclaveLoad for InnerDevice { ..Default::default() }; let createdata = ioctl::CreateData { secs: &secs }; - ioctl_unsafe!( Create, ioctl::create(mapping.device.fd.as_raw_fd(), &createdata) From 707e21e17a31d44c44b3a74a3a2037bb78cd9912 Mon Sep 17 00:00:00 2001 From: Raoul Strackx Date: Thu, 20 Feb 2025 14:23:00 +0100 Subject: [PATCH 3/3] Addressed reviewer comments --- intel-sgx/enclave-runner/src/loader.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/intel-sgx/enclave-runner/src/loader.rs b/intel-sgx/enclave-runner/src/loader.rs index 61735836..baedfec8 100644 --- a/intel-sgx/enclave-runner/src/loader.rs +++ b/intel-sgx/enclave-runner/src/loader.rs @@ -196,16 +196,9 @@ impl<'a> EnclaveBuilder<'a> { return 0; } - let (base, size) = { - let cpuid = cpuid(0x0d, bit); - let base = cpuid.map_or(0, |c| c.ebx); - - if base == 0 { - (0, 0) - } else { - let size = cpuid.map_or(0, |c| c.eax); - (base, size) - } + let CpuidResult { ebx: base, eax: size, .. } = match cpuid(0x0d, bit) { + None | Some(CpuidResult { ebx: 0, .. }) => return 0, + Some(v) => v, }; if max_ssaframesize_in_pages * 0x1000 < base + size {