| Message ID | 20210129001459.1538805-1-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Remove unnecessary kmap() from sgx_ioc_enclave_init() | expand |
On Thu, Jan 28, 2021, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > There is no reason to alloc a page and kmap it to store this temporary > data from the user. Actually, there is, it's just poorly documented. The sigstruct needs to be page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't guarantee alignment. IIRC things will work until slub_debug is enabled, at which point the natural alignment behavior goes out the window. > This is especially true when we are trying to > remove kmap usages. Also placing the token pointer 1/2 way into the > page is fragile. > > Replace this allocation with two kzalloc()'s which also removes the need > for the memset(). > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 90a5caf76939..9c9019760585 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) > { > struct sgx_sigstruct *sigstruct; > struct sgx_enclave_init init_arg; > - struct page *initp_page; > void *token; > int ret; > > @@ -615,13 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) > if (copy_from_user(&init_arg, arg, sizeof(init_arg))) > return -EFAULT; > > - initp_page = alloc_page(GFP_KERNEL); > - if (!initp_page) > + sigstruct = kzalloc(sizeof(*sigstruct), GFP_KERNEL); > + if (!sigstruct) > return -ENOMEM; > > - sigstruct = kmap(initp_page); > - token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); > - memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); > + token = kzalloc(SGX_LAUNCH_TOKEN_SIZE, GFP_KERNEL); > + if (!token) { > + ret = -ENOMEM; > + goto free_sigstruct; > + } > > if (copy_from_user(sigstruct, (void __user *)init_arg.sigstruct, > sizeof(*sigstruct))) { > @@ -645,8 +646,9 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) > ret = sgx_encl_init(encl, sigstruct, token); > > out: > - kunmap(initp_page); > - __free_page(initp_page); > + kfree(token); > +free_sigstruct: > + kfree(sigstruct); > return ret; > } > > -- > 2.28.0.rc0.12.gb6a658bd00c9 >
On Fri, Jan 29, 2021 at 09:37:30AM -0800, Sean Christopherson wrote: > On Thu, Jan 28, 2021, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > There is no reason to alloc a page and kmap it to store this temporary > > data from the user. > > Actually, there is, it's just poorly documented. The sigstruct needs to be > page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't > guarantee alignment. IIRC things will work until slub_debug is enabled, at > which point the natural alignment behavior goes out the window. Well, there still is absolutely no need for the kmap as you can use page_address for a GFP_KERNEL allocation.
On Mon, Feb 01, 2021 at 08:48:12AM +0000, Christoph Hellwig wrote: > On Fri, Jan 29, 2021 at 09:37:30AM -0800, Sean Christopherson wrote: > > On Thu, Jan 28, 2021, ira.weiny@intel.com wrote: > > > From: Ira Weiny <ira.weiny@intel.com> > > > > > > There is no reason to alloc a page and kmap it to store this temporary > > > data from the user. > > > > Actually, there is, it's just poorly documented. The sigstruct needs to be > > page aligned, and the token needs to be 512-byte aligned. kmcalloc doesn't > > guarantee alignment. IIRC things will work until slub_debug is enabled, at > > which point the natural alignment behavior goes out the window. > > Well, there still is absolutely no need for the kmap as you can use > page_address for a GFP_KERNEL allocation. Yeah, we do that in sgx_ioc_enclave_create already based on feedback: secs = kmalloc(PAGE_SIZE, GFP_KERNEL); if (!secs) return -ENOMEM; The kmap() in sgx_ioc_enclave_init() is an unfortunate miss. Let's just follow the pre-existing pattern. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 90a5caf76939..9c9019760585 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -604,7 +604,6 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) { struct sgx_sigstruct *sigstruct; struct sgx_enclave_init init_arg; - struct page *initp_page; void *token; int ret; @@ -615,13 +614,15 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) if (copy_from_user(&init_arg, arg, sizeof(init_arg))) return -EFAULT; - initp_page = alloc_page(GFP_KERNEL); - if (!initp_page) + sigstruct = kzalloc(sizeof(*sigstruct), GFP_KERNEL); + if (!sigstruct) return -ENOMEM; - sigstruct = kmap(initp_page); - token = (void *)((unsigned long)sigstruct + PAGE_SIZE / 2); - memset(token, 0, SGX_LAUNCH_TOKEN_SIZE); + token = kzalloc(SGX_LAUNCH_TOKEN_SIZE, GFP_KERNEL); + if (!token) { + ret = -ENOMEM; + goto free_sigstruct; + } if (copy_from_user(sigstruct, (void __user *)init_arg.sigstruct, sizeof(*sigstruct))) { @@ -645,8 +646,9 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg) ret = sgx_encl_init(encl, sigstruct, token); out: - kunmap(initp_page); - __free_page(initp_page); + kfree(token); +free_sigstruct: + kfree(sigstruct); return ret; }