| Message ID | 20200908190042.24895-1-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES | expand |
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 >
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
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
On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote: > > > 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. > > It is better to have a contract in the API that the number of processed > pages can be less than given, not unlike in syscalls such as write(). That can be handled by a comment, no? If we want to "enforce" the behavior, I'd rather bail out of the loop after a random number of pages than have a completely arbitrary limit. The arbitrary limit will create a contract of its own and may lead to weird guest implementations.
nOn Fri, 11 Sep 2020 10:51:27 -0500, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote: >> On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote: >> > > 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. >> >> It is better to have a contract in the API that the number of processed >> pages can be less than given, not unlike in syscalls such as write(). > > That can be handled by a comment, no? If we want to "enforce" the > behavior, > I'd rather bail out of the loop after a random number of pages than have > a > completely arbitrary limit. The arbitrary limit will create a contract > of > its own and may lead to weird guest implementations. I agree with Sean on potential issues with the arbitrary hard coded limit. Also returning -EINTR is better way to express to user space that operations are interrupted by signal and can be retried, which is a known pattern for this kind of situations.
On Fri, Sep 11, 2020 at 12:38:59PM -0500, Haitao Huang wrote: > nOn Fri, 11 Sep 2020 10:51:27 -0500, Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote: > > > On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote: > > > > > 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. > > > > > > It is better to have a contract in the API that the number of processed > > > pages can be less than given, not unlike in syscalls such as write(). > > > > That can be handled by a comment, no? If we want to "enforce" the > > behavior, > > I'd rather bail out of the loop after a random number of pages than have > > a > > completely arbitrary limit. The arbitrary limit will create a contract > > of > > its own and may lead to weird guest implementations. > > > I agree with Sean on potential issues with the arbitrary hard coded limit. > Also returning -EINTR is better way to express to user space that operations > are interrupted by signal and can be retried, which is a known pattern for > this kind of situations. In read() -EINTR is returned only when zero amount of data is processed. Otherwise, it returns just the count. /Jarkko
On Fri, Sep 11, 2020 at 08:51:27AM -0700, Sean Christopherson wrote: > On Fri, Sep 11, 2020 at 02:43:15PM +0300, Jarkko Sakkinen wrote: > > On Tue, Sep 08, 2020 at 10:30:33PM -0700, Sean Christopherson wrote: > > > > 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. > > > > It is better to have a contract in the API that the number of processed > > pages can be less than given, not unlike in syscalls such as write(). > > That can be handled by a comment, no? If we want to "enforce" the behavior, > I'd rather bail out of the loop after a random number of pages than have a > completely arbitrary limit. The arbitrary limit will create a contract of > its own and may lead to weird guest implementations. I don't understand. It is already a random number given that also signal can cause this. /Jarkko
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
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(-)