arch/x86: Fix size overflows in sgx_encl_create()
Subsystems (3)
Jarkko Sakkinen
maintainerPATCH v2 1/1 arch/x86: Fix size overflows in sgx_encl_create()
The total size calculated for EPC can overflow u64 given the added up page for SECS. Further, the total size calculated for shmem can overflow even when the EPC size stays within limits of u64, given that it adds the extra space for 128 byte PCMD structures (one for each page).
Address this by pre-evaluating the micro-architectural requirement of
SGX: the address space size must be power of two. This is eventually checked up by ECREATE but the pre-check has the additional benefit of making sure that there is some space for additional data.
---
v2: Simply check the micro-architetural requirement in order to address
Dave's comment:
https://lore.kernel.org/linux-sgx/[email protected]/
---
arch/x86/kernel/cpu/sgx/ioctl.c | 8 ++++++++
1 file changed, 8 insertions(+)arch/x86/kernel/cpu/sgx/ioctl.c +8 -0
@@ -64,6 +64,14 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)struct file *backing;long ret; +/* +* This is a micro-architectural requirement. ECREATE would detect this +* too without mentionable overhead but this check guarantees also that +* the space calculations for EPC and shmem allocations never overflow. +*/ +if (!is_power_of_2 (secs->size)) +return -EINVAL ; + va_page =sgx_encl_grow (encl, true);if (IS_ERR (va_page))return PTR_ERR (va_page);
Signature · 1 line
2.48.1
Jarkko Sakkinen
maintainerQuoted 5 lines · Jarkko Sakkinen
The total size calculated for EPC can overflow u64 given the added up page for SECS. Further, the total size calculated for shmem can overflow even when the EPC size stays within limits of u64, given that it adds the extra space for 128 byte PCMD structures (one for each page).
Quoted 8 lines
Address this by pre-evaluating the micro-architectural requirement of
SGX: the address space size must be power of two. This is eventually checked up by ECREATE but the pre-check has the additional benefit of making sure that there is some space for additional data.Reported-by Dan Carpenter <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>
BR, Jarkko
arch/x86/kernel/cpu/sgx/ioctl.c static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) · line 73
Quoted 7 lines · Jarkko Sakkinen
arch/x86/kernel/cpu/sgx/ioctl.c +7
-0+/* +* This is a micro-architectural requirement. ECREATE would detect this +* too without mentionable overhead but this check guarantees also that +* the space calculations for EPC and shmem allocations never overflow. +*/ +if (!is_power_of_2 (secs->size)) +return -EINVAL ;
Isn't it a plain old documented architectural requirement?
Jarkko Sakkinen
maintainerQuoted 10 lines · Dave Hansen
arch/x86/kernel/cpu/sgx/ioctl.c static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) · line 73
Quoted 7 lines · Jarkko Sakkinen
arch/x86/kernel/cpu/sgx/ioctl.c +7
-0+/* +* This is a micro-architectural requirement. ECREATE would detect this +* too without mentionable overhead but this check guarantees also that +* the space calculations for EPC and shmem allocations never overflow. +*/ +if (!is_power_of_2 (secs->size)) +return -EINVAL ;Isn't it a plain old documented architectural requirement?
Yes, but it requires some explanation why it exists here instead of counting on ECREATE.
What do you suggest?
BR, Jarkko
Quoted 10 lines · Jarkko Sakkinen
Quoted 9 lines · Dave Hansen
arch/x86/kernel/cpu/sgx/ioctl.c static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) · line 73
Quoted 7 lines · Jarkko Sakkinen
arch/x86/kernel/cpu/sgx/ioctl.c +7
-0+/* +* This is a micro-architectural requirement. ECREATE would detect this +* too without mentionable overhead but this check guarantees also that +* the space calculations for EPC and shmem allocations never overflow. +*/ +if (!is_power_of_2 (secs->size)) +return -EINVAL ;Isn't it a plain old documented architectural requirement?
Quoted 4 lines
Yes, but it requires some explanation why it exists here instead of counting on ECREATE.
What do you suggest?
/* * ECREATE would detect this too, but checking here also ensures * that the 'encl_size' calculations below can never overflow. */
Jarkko Sakkinen
maintainerQuoted 14 lines · Dave Hansen
Quoted 13 lines · Jarkko Sakkinen
Quoted 9 lines · Dave Hansen
arch/x86/kernel/cpu/sgx/ioctl.c static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) · line 73
Quoted 7 lines · Jarkko Sakkinen
arch/x86/kernel/cpu/sgx/ioctl.c +7
-0+/* +* This is a micro-architectural requirement. ECREATE would detect this +* too without mentionable overhead but this check guarantees also that +* the space calculations for EPC and shmem allocations never overflow. +*/ +if (!is_power_of_2 (secs->size)) +return -EINVAL ;Isn't it a plain old documented architectural requirement?
Yes, but it requires some explanation why it exists here instead of counting on ECREATE.
Quoted 6 lines
Quoted 1 line
What do you suggest?
/* * ECREATE would detect this too, but checking here also ensures * that the 'encl_size' calculations below can never overflow. */
Looks reasonable to me!
BR, Jarkko