| Message ID | 20191105112056.21452-4-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for,v24,v2,1/4] x86/sgx: Destroy enclave if EADD fails | expand |
On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote: > Add @count write the number of bytes added as there is not any good reason > to overwrite input parameters. I disagree, overwriting the params means userspace doesn't need to adjust the values to restart the ioctl(). Ditto for printing out the failing address if the ioctl() fails. > Also, three parameters are unnecessarily > overwritten as the amount of change is the same for each of them. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > arch/x86/include/uapi/asm/sgx.h | 2 ++ > arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++----------- > 2 files changed, 8 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index 88644b6ad849..e196cfd44b70 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -45,6 +45,7 @@ struct sgx_enclave_create { > * @length: length of the data (multiple of the page size) > * @secinfo: address for the SECINFO data > * @flags: page control flags > + * @count: number of bytes added (multiple of the page size) > */ > struct sgx_enclave_add_pages { > __u64 src; > @@ -52,6 +53,7 @@ struct sgx_enclave_add_pages { > __u64 length; > __u64 secinfo; > __u64 flags; > + __u64 count; > }; > > /** > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index deca49bd4f58..e8697d145dfb 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -491,11 +491,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, > * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek > * an address range for the enclave that can be then populated into SECS. > * > - * @arg->addr, @arg->src and @arg->length are adjusted to reflect the > - * remaining pages that need to be added to the enclave, e.g. userspace can > - * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an > - * ERESTARTSYS error. > - * > * Return: > * 0 on success, > * -EACCES if an executable source page is located in a noexec partition, > @@ -506,6 +501,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) > { > struct sgx_enclave_add_pages addp; > struct sgx_secinfo secinfo; > + unsigned long c; > int ret; > > if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED)) > @@ -534,7 +530,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) > if (sgx_validate_secinfo(&secinfo)) > return -EINVAL; > > - for ( ; addp.length > 0; addp.length -= PAGE_SIZE) { > + for (c = 0 ; c < addp.length; c += PAGE_SIZE) { > if (signal_pending(current)) { > ret = -ERESTARTSYS; > break; > @@ -543,15 +539,14 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) > if (need_resched()) > cond_resched(); > > - ret = sgx_encl_add_page(encl, addp.src, addp.offset, > - addp.length, &secinfo, addp.flags); > + ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c, > + addp.length - c, &secinfo, addp.flags); > if (ret) > break; > - > - addp.offset += PAGE_SIZE; > - addp.src += PAGE_SIZE; > } > > + addp.count = c; > + > if (copy_to_user(arg, &addp, sizeof(addp))) > return -EFAULT; > > -- > 2.20.1 >
On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote: > On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote: > > Add @count write the number of bytes added as there is not any good reason > > to overwrite input parameters. > > I disagree, overwriting the params means userspace doesn't need to adjust > the values to restart the ioctl(). Ditto for printing out the failing > address if the ioctl() fails. There is three redundant updates. At least only @length must be updated in order to remove this glitch. As far as overwriting goes, it should be only done when there is requiring to do that. /Jarkko
On Thu, Nov 07, 2019 at 01:20:30AM +0200, Jarkko Sakkinen wrote: > On Tue, Nov 05, 2019 at 02:52:23PM -0800, Sean Christopherson wrote: > > On Tue, Nov 05, 2019 at 01:20:56PM +0200, Jarkko Sakkinen wrote: > > > Add @count write the number of bytes added as there is not any good reason > > > to overwrite input parameters. > > > > I disagree, overwriting the params means userspace doesn't need to adjust > > the values to restart the ioctl(). Ditto for printing out the failing > > address if the ioctl() fails. > > There is three redundant updates. At least only @length must be > updated in order to remove this glitch. > > As far as overwriting goes, it should be only done when there is > requiring to do that. What is obvious is that the current behaviour is wrong. You have a *single value* to return and you encode the *same value* with *three encodings*: 1. offset + count 2. length - count 3. src + count And ironically none of the encodings give you the count of bytes processed in unpacked form. It is something that must be readily available as practically all common syscalls that can partially process the input give that. There is a long history of that pattern and no history at all with this weird pack of encodings. /Jarkko
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index 88644b6ad849..e196cfd44b70 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -45,6 +45,7 @@ struct sgx_enclave_create { * @length: length of the data (multiple of the page size) * @secinfo: address for the SECINFO data * @flags: page control flags + * @count: number of bytes added (multiple of the page size) */ struct sgx_enclave_add_pages { __u64 src; @@ -52,6 +53,7 @@ struct sgx_enclave_add_pages { __u64 length; __u64 secinfo; __u64 flags; + __u64 count; }; /** diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index deca49bd4f58..e8697d145dfb 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -491,11 +491,6 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, * permissions. In effect, this allows mmap() with PROT_NONE to be used to seek * an address range for the enclave that can be then populated into SECS. * - * @arg->addr, @arg->src and @arg->length are adjusted to reflect the - * remaining pages that need to be added to the enclave, e.g. userspace can - * re-invoke SGX_IOC_ENCLAVE_ADD_PAGES using the same struct in response to an - * ERESTARTSYS error. - * * Return: * 0 on success, * -EACCES if an executable source page is located in a noexec partition, @@ -506,6 +501,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) { struct sgx_enclave_add_pages addp; struct sgx_secinfo secinfo; + unsigned long c; int ret; if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED)) @@ -534,7 +530,7 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) if (sgx_validate_secinfo(&secinfo)) return -EINVAL; - for ( ; addp.length > 0; addp.length -= PAGE_SIZE) { + for (c = 0 ; c < addp.length; c += PAGE_SIZE) { if (signal_pending(current)) { ret = -ERESTARTSYS; break; @@ -543,15 +539,14 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) if (need_resched()) cond_resched(); - ret = sgx_encl_add_page(encl, addp.src, addp.offset, - addp.length, &secinfo, addp.flags); + ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c, + addp.length - c, &secinfo, addp.flags); if (ret) break; - - addp.offset += PAGE_SIZE; - addp.src += PAGE_SIZE; } + addp.count = c; + if (copy_to_user(arg, &addp, sizeof(addp))) return -EFAULT;
Add @count write the number of bytes added as there is not any good reason to overwrite input parameters. Also, three parameters are unnecessarily overwritten as the amount of change is the same for each of them. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- arch/x86/include/uapi/asm/sgx.h | 2 ++ arch/x86/kernel/cpu/sgx/ioctl.c | 17 ++++++----------- 2 files changed, 8 insertions(+), 11 deletions(-)