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@xxxxxxxxxxxxxxx>
> ---
> 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
>
