On Tue, Sep 08, 2020 at 10:00:42PM +0300, Jarkko Sakkinen wrote: > Remove 'count' from struct sgx_enclave_add_pages and return number of > pages processed as a positive return as there is no reasonable use for > -EINTR for the caller. Then, roof the processing with a fixed constant > and break out the loop also when this value is surpassed. > > Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@xxxxxxx/ > Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > Cc: Borislav Petkov <bp@xxxxxxx> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx> > --- > arch/x86/include/uapi/asm/sgx.h | 2 -- > arch/x86/kernel/cpu/sgx/ioctl.c | 10 ++++------ > arch/x86/kernel/cpu/sgx/sgx.h | 1 + > 3 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h > index b3d5ccf5b976..1564d7f88597 100644 > --- a/arch/x86/include/uapi/asm/sgx.h > +++ b/arch/x86/include/uapi/asm/sgx.h > @@ -45,7 +45,6 @@ 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; > @@ -53,7 +52,6 @@ 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 6c3c6177b690..0ae00fa9b589 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, > * caller. > * > * Return: > - * 0 on success, > + * length of the data processed on success, > * -EACCES if an executable source page is located in a noexec partition, > * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails > * -errno otherwise > @@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) > return -EINVAL; > > for (c = 0 ; c < addp.length; c += PAGE_SIZE) { > - if (signal_pending(current)) { > - ret = -EINTR; > + if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) { > + ret = c; I don't have an opinion on returning count vs. EINTR, but I don't see the point in arbitrarily capping the number of pages that can be added in a single ioctl(). It doesn't provide any real protection, e.g. userspace can simply restart the ioctl() with updated offsets and continue spamming EADDs. We are relying on other limits, e.g. memcg, rlimits, etc... to reign in malicious/broken userspace. There is nothing inherently dangerous about spending time in the kernel so long as appropriate checks are made, e.g. for a pending signel and resched. If we're missing checks, adding an arbitrary limit won't fix the underlying problem, at least not in a deterministic way. If we really want a limit of some form, adding a knob to control the max size of an enclave seems like the way to go. But even that is of dubious value as I'd rather rely on existing limits for virtual and physical memory, and add a proper EPC cgroup to account and limit EPC memory. > break; > } > > @@ -544,12 +544,10 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg) > break; > } > > - addp.count = c; > - > if (copy_to_user(arg, &addp, sizeof(addp))) > return -EFAULT; > > - return ret; > + return c; > } > > static int __sgx_get_key_hash(struct crypto_shash *tfm, const void *modulus, > diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h > index 1a6ca5f734e5..94b29f378ea2 100644 > --- a/arch/x86/kernel/cpu/sgx/sgx.h > +++ b/arch/x86/kernel/cpu/sgx/sgx.h > @@ -37,6 +37,7 @@ struct sgx_epc_section { > #define SGX_EPC_SECTION_MASK GENMASK(7, 0) > #define SGX_MAX_EPC_SECTIONS (SGX_EPC_SECTION_MASK + 1) > #define SGX_EPC_PAGE_RECLAIMABLE BIT(8) > +#define SGX_MAX_ADD_PAGES_LENGTH 0x100000 > #define SGX_NR_TO_SCAN 16 > #define SGX_NR_LOW_PAGES 32 > #define SGX_NR_HIGH_PAGES 64 > -- > 2.25.1 >
