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
