Re: [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Jarkko Sakkinen <jarkko@kernel.org>]

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