* [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES
@ 2020-09-08 19:00 Jarkko Sakkinen
2020-09-09 5:30 ` Sean Christopherson
2020-09-09 9:55 ` Darren Kenny
0 siblings, 2 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2020-09-08 19:00 UTC (permalink / raw)
To: linux-sgx; +Cc: Jarkko Sakkinen, Sean Christopherson, Borislav Petkov
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@zn.tnic/
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Borislav Petkov <bp@suse.de>
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 | 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;
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES
2020-09-08 19:00 [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
@ 2020-09-09 5:30 ` Sean Christopherson
[not found] ` <20200911114315.GA6760@linux.intel.com>
2020-09-09 9:55 ` Darren Kenny
1 sibling, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2020-09-09 5:30 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-sgx, Borislav Petkov
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@zn.tnic/
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> 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 | 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES
2020-09-08 19:00 [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-09-09 5:30 ` Sean Christopherson
@ 2020-09-09 9:55 ` Darren Kenny
2020-09-11 11:50 ` Jarkko Sakkinen
1 sibling, 1 reply; 8+ messages in thread
From: Darren Kenny @ 2020-09-09 9:55 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-sgx
Cc: Jarkko Sakkinen, Sean Christopherson, Borislav Petkov
On Tuesday, 2020-09-08 at 22:00:42 +03, 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@zn.tnic/
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> 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 | 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)) {
I know that SGX_MAX_ADD_PAGES_LENGTH may be a multiple of PAGE_SIZE, but
to be sure that nothing breaks here in the future, I think it would be
better to use '>=' as the comparison rather than just '=='.
Also, the indentation seems off here w.r.t. using TABs when the break
below is using spaces - would be good to have them the same at least.
Thanks,
Darren.
> + ret = c;
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES
2020-09-09 9:55 ` Darren Kenny
@ 2020-09-11 11:50 ` Jarkko Sakkinen
0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Sakkinen @ 2020-09-11 11:50 UTC (permalink / raw)
To: Darren Kenny; +Cc: linux-sgx, Sean Christopherson, Borislav Petkov
On Wed, Sep 09, 2020 at 10:55:58AM +0100, Darren Kenny wrote:
> On Tuesday, 2020-09-08 at 22:00:42 +03, 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@zn.tnic/
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > 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 | 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)) {
>
> I know that SGX_MAX_ADD_PAGES_LENGTH may be a multiple of PAGE_SIZE, but
> to be sure that nothing breaks here in the future, I think it would be
> better to use '>=' as the comparison rather than just '=='.
I'm not forseeing this. We will change the comparison or remove it if
the constant is no longer required.
>
> Also, the indentation seems off here w.r.t. using TABs when the break
> below is using spaces - would be good to have them the same at least.
Ugh, that must be my mistake. I'll check these before squashing this.
Thank you.
/Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-14 18:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 19:00 [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-09-09 5:30 ` Sean Christopherson
[not found] ` <20200911114315.GA6760@linux.intel.com>
2020-09-11 15:51 ` Sean Christopherson
2020-09-11 17:38 ` Haitao Huang
2020-09-14 18:07 ` Jarkko Sakkinen
2020-09-14 18:09 ` Jarkko Sakkinen
2020-09-09 9:55 ` Darren Kenny
2020-09-11 11:50 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).