* [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
@ 2025-03-04 13:05 Dan Carpenter
2025-03-04 22:07 ` Jarkko Sakkinen
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-03-04 13:05 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-sgx
Hello Jarkko Sakkinen,
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'
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.
76
77 backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Also this can integer overflow.
78 VM_NORESERVE);
79 if (IS_ERR(backing)) {
80 ret = PTR_ERR(backing);
81 goto err_out_shrink;
82 }
83
84 encl->backing = backing;
85
86 secs_epc = sgx_alloc_epc_page(&encl->secs, true);
87 if (IS_ERR(secs_epc)) {
88 ret = PTR_ERR(secs_epc);
89 goto err_out_backing;
90 }
91
92 encl->secs.epc_page = secs_epc;
93
94 pginfo.addr = 0;
95 pginfo.contents = (unsigned long)secs;
96 pginfo.metadata = (unsigned long)&secinfo;
97 pginfo.secs = 0;
98 memset(&secinfo, 0, sizeof(secinfo));
99
100 ret = __ecreate((void *)&pginfo, sgx_get_epc_virt_addr(secs_epc));
101 if (ret) {
102 ret = -EIO;
103 goto err_out;
104 }
105
106 if (secs->attributes & SGX_ATTR_DEBUG)
107 set_bit(SGX_ENCL_DEBUG, &encl->flags);
108
109 encl->secs.encl = encl;
110 encl->secs.type = SGX_PAGE_TYPE_SECS;
111 encl->base = secs->base;
112 encl->size = secs->size;
^^^^^^^^^^^^^^^^^^^^^^^
Then we re-use secs->size here. I haven't followed this code through to
see if anything bad happens, but it makes me itch just looking at it. :P
113 encl->attributes = secs->attributes;
114 encl->attributes_mask = SGX_ATTR_UNPRIV_MASK;
115
116 /* Set only after completion, as encl->lock has not been taken. */
117 set_bit(SGX_ENCL_CREATED, &encl->flags);
118
119 return 0;
120
121 err_out:
122 sgx_encl_free_epc_page(encl->secs.epc_page);
123 encl->secs.epc_page = NULL;
124
125 err_out_backing:
126 fput(encl->backing);
127 encl->backing = NULL;
128
129 err_out_shrink:
130 sgx_encl_shrink(encl, va_page);
131
132 return ret;
133 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
2025-03-04 13:05 [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Dan Carpenter
@ 2025-03-04 22:07 ` Jarkko Sakkinen
2025-03-05 7:44 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jarkko Sakkinen @ 2025-03-04 22:07 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-sgx
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
2025-03-04 22:07 ` Jarkko Sakkinen
@ 2025-03-05 7:44 ` Dan Carpenter
2025-03-06 18:28 ` Jarkko Sakkinen
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-03-05 7:44 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-sgx
On Wed, Mar 05, 2025 at 12:07:27AM +0200, Jarkko Sakkinen wrote:
> 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?
You're doing it correctly. I don't know what "Error 139" means... I'm not
able reproduce this.
> I'd just love to learn how to use it in
> order to make sure that my fix will address the bug.
This is something I'm still working on so it's not published yet.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE
2025-03-05 7:44 ` Dan Carpenter
@ 2025-03-06 18:28 ` Jarkko Sakkinen
0 siblings, 0 replies; 4+ messages in thread
From: Jarkko Sakkinen @ 2025-03-06 18:28 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-sgx
On Wed, 2025-03-05 at 10:44 +0300, Dan Carpenter wrote:
> > I'd just love to learn how to use it in
> > order to make sure that my fix will address the bug.
>
> This is something I'm still working on so it's not published yet.
OK, if you put out anything related to the this tool to LKML, please
CC me. And thanks again for reporting the bug!
>
> regards,
> dan carpenter
>
BR, Jarkko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-06 18:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-04 13:05 [bug report] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Dan Carpenter
2025-03-04 22:07 ` Jarkko Sakkinen
2025-03-05 7:44 ` Dan Carpenter
2025-03-06 18:28 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox