On Tue, Mar 04, 2025 at 04:05:45PM +0300, Dan Carpenter wrote:
> Hello Jarkko Sakkinen,
Hi Dan, thanks for the report, it is very well put together with all the
detail it has :-)
>
> Commit 888d24911787 ("x86/sgx: Add SGX_IOC_ENCLAVE_CREATE") from Nov
> 13, 2020 (linux-next), leads to the following Smatch static checker
> warning:
>
> arch/x86/kernel/cpu/sgx/ioctl.c:75 sgx_encl_create()
> warn: potential user controlled sizeof overflow 'secs->size + ((1) <<
> 12)' '0-u64max + 4096'
What I tried:
1. Installed smatch package to Fedora.
2. Cloned git clone git://repo.or.cz/smatch.git
I get:
$ O=.clangd ~/work/staging/smatch/smatch_scripts/kchecker
arch/x86/kernel/cpu/sgx/ioctl.c
make[1]: Entering directory
'/home/jarkko/work/kernel.org/jarkko/linux-tpmdd/.clangd'
SYNC include/config/auto.conf
GEN Makefile
GEN Makefile
CHECK ../scripts/mod/empty.c
CALL ../scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CHECK ../arch/x86/kernel/cpu/sgx/ioctl.c
make[7]: *** [../scripts/Makefile.build:208: arch/x86/kernel/cpu/sgx/ioctl.o]
Error 139
make[6]: *** [../scripts/Makefile.build:465: arch/x86/kernel/cpu/sgx] Error 2
make[5]: *** [../scripts/Makefile.build:465: arch/x86/kernel/cpu] Error 2
make[4]: *** [../scripts/Makefile.build:465: arch/x86/kernel] Error 2
make[3]: *** [../scripts/Makefile.build:465: arch/x86] Error 2
make[2]: *** [/home/jarkko/work/kernel.org/jarkko/linux-tpmdd/Makefile:1989: .]
Error 2
make[1]: *** [/home/jarkko/work/kernel.org/jarkko/linux-tpmdd/Makefile:251:
__sub-make] Error 2
make[1]: Leaving directory
'/home/jarkko/work/kernel.org/jarkko/linux-tpmdd/.clangd'
make: *** [Makefile:251: __sub-make] Error 2
Am I using smatch incorrectly? I'd just love to learn how to use it in
order to make sure that my fix will address the bug.
>
> arch/x86/kernel/cpu/sgx/ioctl.c
> 57 static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs
> *secs)
> 58 {
> 59 struct sgx_epc_page *secs_epc;
> 60 struct sgx_va_page *va_page;
> 61 struct sgx_pageinfo pginfo;
> 62 struct sgx_secinfo secinfo;
> 63 unsigned long encl_size;
> 64 struct file *backing;
> 65 long ret;
> 66
> 67 va_page = sgx_encl_grow(encl, true);
> 68 if (IS_ERR(va_page))
> 69 return PTR_ERR(va_page);
> 70 else if (va_page)
> 71 list_add(&va_page->list, &encl->va_pages);
> 72 /* else the tail page of the VA page list had free slots. */
> 73
> 74 /* The extra page goes to SECS. */
> --> 75 encl_size = secs->size + PAGE_SIZE;
>
> secs->size is a u64 that comes from the user in sgx_ioc_enclave_create().
> If we add PAGE_SIZE that could be an integer overflow. Also encl_size is
> unsigned long so on 32 bits that's another integer overflow.
It's guaranteeed to be 64-bit given:
config X86_SGX
bool "Software Guard eXtensions (SGX)"
depends on X86_64 && CPU_SUP_INTEL && X86_X2APIC
That said, it would add clarity to explicitly declare encl_size as u64.
How it is now, is somewhat obfuscated, I give you that (but still not
incorrect).
>
> 76
> 77 backing = shmem_file_setup("SGX backing", encl_size +
> (encl_size >> 5),
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Also this can integer overflow.
I drafted this quickly but I think we could probably fix this up by:
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
index b65ab214bdf5..551e65c94eb3 100644
--- a/arch/x86/kernel/cpu/sgx/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/ioctl.c
@@ -60,10 +60,20 @@ static int sgx_encl_create(struct sgx_encl *encl, struct
sgx_secs *secs)
struct sgx_va_page *va_page;
struct sgx_pageinfo pginfo;
struct sgx_secinfo secinfo;
- unsigned long encl_size;
+ u64 encl_size, pcmd_size;
struct file *backing;
long ret;
+ if ((u64)PAGE_SIZE > ~secs->size)
+ return -E2BIG;
+
+ /* The extra page is for SECS: */
+ encl_size = secs->size + PAGE_SIZE;
+ pcmd_size = encl_size >> 5;
+
+ if (pcmd_size > ~encl_size)
+ return -E2BIG;
+
va_page = sgx_encl_grow(encl, true);
if (IS_ERR(va_page))
return PTR_ERR(va_page);
@@ -71,9 +81,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct
sgx_secs *secs)
list_add(&va_page->list, &encl->va_pages);
/* else the tail page of the VA page list had free slots. */
- /* The extra page goes to SECS. */
- encl_size = secs->size + PAGE_SIZE;
-
backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
VM_NORESERVE);
if (IS_ERR(backing)) {
The gist here is that by inverting bits we check if the remaining space
in 64-bit number has enough space for the "extra fill"
BR, Jarkko
