[intel-sgx-kernel-dev] intel_sgx: Fix max enclave size calculation
diff mbox

Message ID 1497650241-17149-1-git-send-email-angie.v.chinchilla@intel.com
State New
Headers show

Commit Message

Angie Chinchilla June 16, 2017, 9:57 p.m. UTC
sgx_encl_size_max_64 and sgx_encl_size_max_32 calculations are
incorrect, causing failures creating large enclaves.
2^EDX[7:0] should be used for max enclave size in 32-bit mode
2^EDX[15:8] should be used for max enclave size in 64-bit mode

Signed-off-by: Angie Chinchilla <angie.v.chinchilla@intel.com>
---
 drivers/platform/x86/intel_sgx/sgx_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jarkko Sakkinen June 19, 2017, 12:18 a.m. UTC | #1
These constants are legacy stuff that should be removed. Would make
more sense to just delete these constants and return -ENODEV if the
cpuid does not give what we want.

/Jarkko

On Fri, 2017-06-16 at 17:57 -0400, Angie Chinchilla wrote:
> sgx_encl_size_max_64 and sgx_encl_size_max_32 calculations are
> incorrect, causing failures creating large enclaves.
> 2^EDX[7:0] should be used for max enclave size in 32-bit mode
> 2^EDX[15:8] should be used for max enclave size in 64-bit mode
> 
> Signed-off-by: Angie Chinchilla <angie.v.chinchilla@intel.com>
> ---
>  drivers/platform/x86/intel_sgx/sgx_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
> index cf1e6ec..1840f81 100644
> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
> @@ -312,9 +312,9 @@ static int sgx_drv_probe(struct platform_device *pdev)
>  	cpuid_count(SGX_CPUID, 0x0, &eax, &ebx, &ecx, &edx);
>  	if (edx & 0xFFFF) {
>  #ifdef CONFIG_X86_64
> -		sgx_encl_size_max_64 = 2ULL << (edx & 0xFF);
> +		sgx_encl_size_max_64 = 2ULL << ((edx >> 8) & 0xFF);
>  #endif
> -		sgx_encl_size_max_32 = 2ULL << ((edx >> 8) & 0xFF);
> +		sgx_encl_size_max_32 = 2ULL << (edx & 0xFF);
>  	}
>  
>  	return sgx_dev_init(&pdev->dev);
Jethro Beekman June 19, 2017, 3:43 p.m. UTC | #2
On 2017-06-18 17:18, Jarkko Sakkinen wrote:
> These constants are legacy stuff that should be removed. Would make
> more sense to just delete these constants and return -ENODEV if the
> cpuid does not give what we want.

Is there any documentation about this being legacy? The March 2017 SDM 
says this code is correct.

Jethro Beekman | Fortanix

> 
> /Jarkko
> 
> On Fri, 2017-06-16 at 17:57 -0400, Angie Chinchilla wrote:
>> sgx_encl_size_max_64 and sgx_encl_size_max_32 calculations are
>> incorrect, causing failures creating large enclaves.
>> 2^EDX[7:0] should be used for max enclave size in 32-bit mode
>> 2^EDX[15:8] should be used for max enclave size in 64-bit mode
>>
>> Signed-off-by: Angie Chinchilla <angie.v.chinchilla@intel.com>
>> ---
>>   drivers/platform/x86/intel_sgx/sgx_main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
>> index cf1e6ec..1840f81 100644
>> --- a/drivers/platform/x86/intel_sgx/sgx_main.c
>> +++ b/drivers/platform/x86/intel_sgx/sgx_main.c
>> @@ -312,9 +312,9 @@ static int sgx_drv_probe(struct platform_device *pdev)
>>   	cpuid_count(SGX_CPUID, 0x0, &eax, &ebx, &ecx, &edx);
>>   	if (edx & 0xFFFF) {
>>   #ifdef CONFIG_X86_64
>> -		sgx_encl_size_max_64 = 2ULL << (edx & 0xFF);
>> +		sgx_encl_size_max_64 = 2ULL << ((edx >> 8) & 0xFF);
>>   #endif
>> -		sgx_encl_size_max_32 = 2ULL << ((edx >> 8) & 0xFF);
>> +		sgx_encl_size_max_32 = 2ULL << (edx & 0xFF);
>>   	}
>>   
>>   	return sgx_dev_init(&pdev->dev);
> _______________________________________________
> intel-sgx-kernel-dev mailing list
> intel-sgx-kernel-dev@lists.01.org
> https://lists.01.org/mailman/listinfo/intel-sgx-kernel-dev
>
Jarkko Sakkinen June 19, 2017, 9:54 p.m. UTC | #3
On Mon, Jun 19, 2017 at 08:43:30AM -0700, Jethro Beekman wrote:
> On 2017-06-18 17:18, Jarkko Sakkinen wrote:
> > These constants are legacy stuff that should be removed. Would make
> > more sense to just delete these constants and return -ENODEV if the
> > cpuid does not give what we want.
> 
> Is there any documentation about this being legacy? The March 2017 SDM says
> this code is correct.

What I was trying to say is that it woud make sense to

if (!(edx & 0xffff)) {
	dev_err(pdev->dev, ...);
	return -ENODEV;
}


Not that the fix itself is improper.

> Jethro Beekman | Fortanix

/Jarkko

Patch
diff mbox

diff --git a/drivers/platform/x86/intel_sgx/sgx_main.c b/drivers/platform/x86/intel_sgx/sgx_main.c
index cf1e6ec..1840f81 100644
--- a/drivers/platform/x86/intel_sgx/sgx_main.c
+++ b/drivers/platform/x86/intel_sgx/sgx_main.c
@@ -312,9 +312,9 @@  static int sgx_drv_probe(struct platform_device *pdev)
 	cpuid_count(SGX_CPUID, 0x0, &eax, &ebx, &ecx, &edx);
 	if (edx & 0xFFFF) {
 #ifdef CONFIG_X86_64
-		sgx_encl_size_max_64 = 2ULL << (edx & 0xFF);
+		sgx_encl_size_max_64 = 2ULL << ((edx >> 8) & 0xFF);
 #endif
-		sgx_encl_size_max_32 = 2ULL << ((edx >> 8) & 0xFF);
+		sgx_encl_size_max_32 = 2ULL << (edx & 0xFF);
 	}
 
 	return sgx_dev_init(&pdev->dev);