* [PATCH] exec: Account for argv/envp pointers
@ 2017-06-22 0:17 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2017-06-22 0:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Alexander Viro, Qualys Security Advisory, linux-mm,
linux-fsdevel, linux-kernel, kernel-hardening
When limiting the argv/envp strings during exec to 1/4 of the stack limit,
the storage of the pointers to the strings was not included. This means
that an exec with huge numbers of tiny strings could eat 1/4 of the
stack limit in strings and then additional space would be later used
by the pointers to the strings. For example, on 32-bit with a 8MB stack
rlimit, an exec with 1677721 single-byte strings would consume less than
2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the
strings would consume the remaining additional stack space (1677721 *
4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust
stack space entirely. Controlling this stack exhaustion could result in
pathological behavior in setuid binaries (CVE-2017-1000365).
Fixes: b6a2fea39318 ("mm: variable length argument support")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/exec.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 72934df68471..8079ca70cfda 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
if (write) {
unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
+ unsigned long ptr_size;
struct rlimit *rlim;
+ /*
+ * Since the stack will hold pointers to the strings, we
+ * must account for them as well.
+ */
+ ptr_size = (bprm->argc + bprm->envc) * sizeof(void *);
+ if (ptr_size > ULONG_MAX - size)
+ goto fail;
+ size += ptr_size;
+
acct_arg_size(bprm, size / PAGE_SIZE);
/*
@@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
* to work from.
*/
rlim = current->signal->rlim;
- if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) {
- put_page(page);
- return NULL;
- }
+ if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4)
+ goto fail;
}
return page;
+
+fail:
+ put_page(page);
+ return NULL;
}
static void put_arg_page(struct page *page)
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 14+ messages in thread* [kernel-hardening] [PATCH] exec: Account for argv/envp pointers @ 2017-06-22 0:17 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-06-22 0:17 UTC (permalink / raw) To: Andrew Morton Cc: Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening When limiting the argv/envp strings during exec to 1/4 of the stack limit, the storage of the pointers to the strings was not included. This means that an exec with huge numbers of tiny strings could eat 1/4 of the stack limit in strings and then additional space would be later used by the pointers to the strings. For example, on 32-bit with a 8MB stack rlimit, an exec with 1677721 single-byte strings would consume less than 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the strings would consume the remaining additional stack space (1677721 * 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust stack space entirely. Controlling this stack exhaustion could result in pathological behavior in setuid binaries (CVE-2017-1000365). Fixes: b6a2fea39318 ("mm: variable length argument support") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/exec.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 72934df68471..8079ca70cfda 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, if (write) { unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; + unsigned long ptr_size; struct rlimit *rlim; + /* + * Since the stack will hold pointers to the strings, we + * must account for them as well. + */ + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); + if (ptr_size > ULONG_MAX - size) + goto fail; + size += ptr_size; + acct_arg_size(bprm, size / PAGE_SIZE); /* @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, * to work from. */ rlim = current->signal->rlim; - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { - put_page(page); - return NULL; - } + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) + goto fail; } return page; + +fail: + put_page(page); + return NULL; } static void put_arg_page(struct page *page) -- 2.7.4 -- Kees Cook Pixel Security ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] exec: Account for argv/envp pointers @ 2017-06-22 0:17 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-06-22 0:17 UTC (permalink / raw) To: Andrew Morton Cc: Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening When limiting the argv/envp strings during exec to 1/4 of the stack limit, the storage of the pointers to the strings was not included. This means that an exec with huge numbers of tiny strings could eat 1/4 of the stack limit in strings and then additional space would be later used by the pointers to the strings. For example, on 32-bit with a 8MB stack rlimit, an exec with 1677721 single-byte strings would consume less than 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the strings would consume the remaining additional stack space (1677721 * 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust stack space entirely. Controlling this stack exhaustion could result in pathological behavior in setuid binaries (CVE-2017-1000365). Fixes: b6a2fea39318 ("mm: variable length argument support") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/exec.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 72934df68471..8079ca70cfda 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, if (write) { unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; + unsigned long ptr_size; struct rlimit *rlim; + /* + * Since the stack will hold pointers to the strings, we + * must account for them as well. + */ + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); + if (ptr_size > ULONG_MAX - size) + goto fail; + size += ptr_size; + acct_arg_size(bprm, size / PAGE_SIZE); /* @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, * to work from. */ rlim = current->signal->rlim; - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { - put_page(page); - return NULL; - } + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) + goto fail; } return page; + +fail: + put_page(page); + return NULL; } static void put_arg_page(struct page *page) -- 2.7.4 -- Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [kernel-hardening] [PATCH] exec: Account for argv/envp pointers 2017-06-22 0:17 ` Kees Cook @ 2017-06-22 1:39 ` Rik van Riel -1 siblings, 0 replies; 14+ messages in thread From: Rik van Riel @ 2017-06-22 1:39 UTC (permalink / raw) To: Kees Cook, Andrew Morton Cc: Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening On Wed, 2017-06-21 at 17:17 -0700, Kees Cook wrote: > When limiting the argv/envp strings during exec to 1/4 of the stack > limit, > the storage of the pointers to the strings was not included. This > means > that an exec with huge numbers of tiny strings could eat 1/4 of the > stack limit in strings and then additional space would be later used > by the pointers to the strings. For example, on 32-bit with a 8MB > stack > rlimit, an exec with 1677721 single-byte strings would consume less > than > 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to > the > strings would consume the remaining additional stack space (1677721 * > 4 == 6710884). The result (1677721 + 6710884 == 8388605) would > exhaust > stack space entirely. Controlling this stack exhaustion could result > in > pathological behavior in setuid binaries (CVE-2017-1000365). > > Fixes: b6a2fea39318 ("mm: variable length argument support") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Rik van Riel <riel@redhat.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [kernel-hardening] [PATCH] exec: Account for argv/envp pointers @ 2017-06-22 1:39 ` Rik van Riel 0 siblings, 0 replies; 14+ messages in thread From: Rik van Riel @ 2017-06-22 1:39 UTC (permalink / raw) To: Kees Cook, Andrew Morton Cc: Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening On Wed, 2017-06-21 at 17:17 -0700, Kees Cook wrote: > When limiting the argv/envp strings during exec to 1/4 of the stack > limit, > the storage of the pointers to the strings was not included. This > means > that an exec with huge numbers of tiny strings could eat 1/4 of the > stack limit in strings and then additional space would be later used > by the pointers to the strings. For example, on 32-bit with a 8MB > stack > rlimit, an exec with 1677721 single-byte strings would consume less > than > 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to > the > strings would consume the remaining additional stack space (1677721 * > 4 == 6710884). The result (1677721 + 6710884 == 8388605) would > exhaust > stack space entirely. Controlling this stack exhaustion could result > in > pathological behavior in setuid binaries (CVE-2017-1000365). > > Fixes: b6a2fea39318 ("mm: variable length argument support") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Rik van Riel <riel@redhat.com> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers 2017-06-22 0:17 ` Kees Cook (?) @ 2017-06-23 13:59 ` Michal Hocko -1 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 13:59 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening On Wed 21-06-17 17:17:20, Kees Cook wrote: > When limiting the argv/envp strings during exec to 1/4 of the stack limit, > the storage of the pointers to the strings was not included. This means > that an exec with huge numbers of tiny strings could eat 1/4 of the > stack limit in strings and then additional space would be later used > by the pointers to the strings. For example, on 32-bit with a 8MB stack > rlimit, an exec with 1677721 single-byte strings would consume less than > 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the > strings would consume the remaining additional stack space (1677721 * > 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust > stack space entirely. Controlling this stack exhaustion could result in > pathological behavior in setuid binaries (CVE-2017-1000365). > > Fixes: b6a2fea39318 ("mm: variable length argument support") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 72934df68471..8079ca70cfda 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > > if (write) { > unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > + unsigned long ptr_size; > struct rlimit *rlim; > > + /* > + * Since the stack will hold pointers to the strings, we > + * must account for them as well. > + */ > + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > + if (ptr_size > ULONG_MAX - size) > + goto fail; > + size += ptr_size; > + > acct_arg_size(bprm, size / PAGE_SIZE); Doesn't this over account? I mean this gets called for partial arguments as they fit into a page so a single argument can get into this function multiple times AFAIU. I also do not understand why would you want to account bprm->argc + bprm->envc pointers for each argument. > > /* > @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > * to work from. > */ > rlim = current->signal->rlim; > - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { > - put_page(page); > - return NULL; > - } > + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) > + goto fail; > } > > return page; > + > +fail: > + put_page(page); > + return NULL; > } > > static void put_arg_page(struct page *page) > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* [kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 13:59 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 13:59 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening On Wed 21-06-17 17:17:20, Kees Cook wrote: > When limiting the argv/envp strings during exec to 1/4 of the stack limit, > the storage of the pointers to the strings was not included. This means > that an exec with huge numbers of tiny strings could eat 1/4 of the > stack limit in strings and then additional space would be later used > by the pointers to the strings. For example, on 32-bit with a 8MB stack > rlimit, an exec with 1677721 single-byte strings would consume less than > 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the > strings would consume the remaining additional stack space (1677721 * > 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust > stack space entirely. Controlling this stack exhaustion could result in > pathological behavior in setuid binaries (CVE-2017-1000365). > > Fixes: b6a2fea39318 ("mm: variable length argument support") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 72934df68471..8079ca70cfda 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > > if (write) { > unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > + unsigned long ptr_size; > struct rlimit *rlim; > > + /* > + * Since the stack will hold pointers to the strings, we > + * must account for them as well. > + */ > + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > + if (ptr_size > ULONG_MAX - size) > + goto fail; > + size += ptr_size; > + > acct_arg_size(bprm, size / PAGE_SIZE); Doesn't this over account? I mean this gets called for partial arguments as they fit into a page so a single argument can get into this function multiple times AFAIU. I also do not understand why would you want to account bprm->argc + bprm->envc pointers for each argument. > > /* > @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > * to work from. > */ > rlim = current->signal->rlim; > - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { > - put_page(page); > - return NULL; > - } > + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) > + goto fail; > } > > return page; > + > +fail: > + put_page(page); > + return NULL; > } > > static void put_arg_page(struct page *page) > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 13:59 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 13:59 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, linux-mm, linux-fsdevel, linux-kernel, kernel-hardening On Wed 21-06-17 17:17:20, Kees Cook wrote: > When limiting the argv/envp strings during exec to 1/4 of the stack limit, > the storage of the pointers to the strings was not included. This means > that an exec with huge numbers of tiny strings could eat 1/4 of the > stack limit in strings and then additional space would be later used > by the pointers to the strings. For example, on 32-bit with a 8MB stack > rlimit, an exec with 1677721 single-byte strings would consume less than > 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the > strings would consume the remaining additional stack space (1677721 * > 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust > stack space entirely. Controlling this stack exhaustion could result in > pathological behavior in setuid binaries (CVE-2017-1000365). > > Fixes: b6a2fea39318 ("mm: variable length argument support") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 72934df68471..8079ca70cfda 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > > if (write) { > unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > + unsigned long ptr_size; > struct rlimit *rlim; > > + /* > + * Since the stack will hold pointers to the strings, we > + * must account for them as well. > + */ > + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > + if (ptr_size > ULONG_MAX - size) > + goto fail; > + size += ptr_size; > + > acct_arg_size(bprm, size / PAGE_SIZE); Doesn't this over account? I mean this gets called for partial arguments as they fit into a page so a single argument can get into this function multiple times AFAIU. I also do not understand why would you want to account bprm->argc + bprm->envc pointers for each argument. > > /* > @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > * to work from. > */ > rlim = current->signal->rlim; > - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { > - put_page(page); > - return NULL; > - } > + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) > + goto fail; > } > > return page; > + > +fail: > + put_page(page); > + return NULL; > } > > static void put_arg_page(struct page *page) > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers 2017-06-23 13:59 ` Michal Hocko (?) @ 2017-06-23 14:05 ` Kees Cook -1 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-06-23 14:05 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Wed 21-06-17 17:17:20, Kees Cook wrote: >> When limiting the argv/envp strings during exec to 1/4 of the stack limit, >> the storage of the pointers to the strings was not included. This means >> that an exec with huge numbers of tiny strings could eat 1/4 of the >> stack limit in strings and then additional space would be later used >> by the pointers to the strings. For example, on 32-bit with a 8MB stack >> rlimit, an exec with 1677721 single-byte strings would consume less than >> 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the >> strings would consume the remaining additional stack space (1677721 * >> 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust >> stack space entirely. Controlling this stack exhaustion could result in >> pathological behavior in setuid binaries (CVE-2017-1000365). >> >> Fixes: b6a2fea39318 ("mm: variable length argument support") >> Cc: stable@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> fs/exec.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 72934df68471..8079ca70cfda 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> >> if (write) { >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; >> + unsigned long ptr_size; >> struct rlimit *rlim; >> >> + /* >> + * Since the stack will hold pointers to the strings, we >> + * must account for them as well. >> + */ >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); >> + if (ptr_size > ULONG_MAX - size) >> + goto fail; >> + size += ptr_size; >> + >> acct_arg_size(bprm, size / PAGE_SIZE); > > Doesn't this over account? I mean this gets called for partial arguments > as they fit into a page so a single argument can get into this function > multiple times AFAIU. I also do not understand why would you want to > account bprm->argc + bprm->envc pointers for each argument. Based on what I could understand in acct_arg_size(), this is called repeatedly with with the "current" size (it handles the difference between prior calls, see calls like acct_arg_size(bprm, 0)). The size calculation is the entire vma while each arg page is built, so each time we get here it's calculating how far it is currently (rather than each call being just the newly added size from the arg page). As a result, we need to always add the entire size of the pointers, so that on the last call to get_arg_page() we'll actually have the entire correct size. -Kees > >> >> /* >> @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> * to work from. >> */ >> rlim = current->signal->rlim; >> - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { >> - put_page(page); >> - return NULL; >> - } >> + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) >> + goto fail; >> } >> >> return page; >> + >> +fail: >> + put_page(page); >> + return NULL; >> } >> >> static void put_arg_page(struct page *page) >> -- >> 2.7.4 >> >> >> -- >> Kees Cook >> Pixel Security >> >> -- >> To unsubscribe, send a message with 'unsubscribe linux-mm' in >> the body to majordomo@kvack.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Michal Hocko > SUSE Labs -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* [kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 14:05 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-06-23 14:05 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Wed 21-06-17 17:17:20, Kees Cook wrote: >> When limiting the argv/envp strings during exec to 1/4 of the stack limit, >> the storage of the pointers to the strings was not included. This means >> that an exec with huge numbers of tiny strings could eat 1/4 of the >> stack limit in strings and then additional space would be later used >> by the pointers to the strings. For example, on 32-bit with a 8MB stack >> rlimit, an exec with 1677721 single-byte strings would consume less than >> 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the >> strings would consume the remaining additional stack space (1677721 * >> 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust >> stack space entirely. Controlling this stack exhaustion could result in >> pathological behavior in setuid binaries (CVE-2017-1000365). >> >> Fixes: b6a2fea39318 ("mm: variable length argument support") >> Cc: stable@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> fs/exec.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 72934df68471..8079ca70cfda 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> >> if (write) { >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; >> + unsigned long ptr_size; >> struct rlimit *rlim; >> >> + /* >> + * Since the stack will hold pointers to the strings, we >> + * must account for them as well. >> + */ >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); >> + if (ptr_size > ULONG_MAX - size) >> + goto fail; >> + size += ptr_size; >> + >> acct_arg_size(bprm, size / PAGE_SIZE); > > Doesn't this over account? I mean this gets called for partial arguments > as they fit into a page so a single argument can get into this function > multiple times AFAIU. I also do not understand why would you want to > account bprm->argc + bprm->envc pointers for each argument. Based on what I could understand in acct_arg_size(), this is called repeatedly with with the "current" size (it handles the difference between prior calls, see calls like acct_arg_size(bprm, 0)). The size calculation is the entire vma while each arg page is built, so each time we get here it's calculating how far it is currently (rather than each call being just the newly added size from the arg page). As a result, we need to always add the entire size of the pointers, so that on the last call to get_arg_page() we'll actually have the entire correct size. -Kees > >> >> /* >> @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> * to work from. >> */ >> rlim = current->signal->rlim; >> - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { >> - put_page(page); >> - return NULL; >> - } >> + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) >> + goto fail; >> } >> >> return page; >> + >> +fail: >> + put_page(page); >> + return NULL; >> } >> >> static void put_arg_page(struct page *page) >> -- >> 2.7.4 >> >> >> -- >> Kees Cook >> Pixel Security >> >> -- >> To unsubscribe, send a message with 'unsubscribe linux-mm' in >> the body to majordomo@kvack.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Michal Hocko > SUSE Labs -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 14:05 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2017-06-23 14:05 UTC (permalink / raw) To: Michal Hocko Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: > On Wed 21-06-17 17:17:20, Kees Cook wrote: >> When limiting the argv/envp strings during exec to 1/4 of the stack limit, >> the storage of the pointers to the strings was not included. This means >> that an exec with huge numbers of tiny strings could eat 1/4 of the >> stack limit in strings and then additional space would be later used >> by the pointers to the strings. For example, on 32-bit with a 8MB stack >> rlimit, an exec with 1677721 single-byte strings would consume less than >> 2MB of stack, the max (8MB / 4) amount allowed, but the pointers to the >> strings would consume the remaining additional stack space (1677721 * >> 4 == 6710884). The result (1677721 + 6710884 == 8388605) would exhaust >> stack space entirely. Controlling this stack exhaustion could result in >> pathological behavior in setuid binaries (CVE-2017-1000365). >> >> Fixes: b6a2fea39318 ("mm: variable length argument support") >> Cc: stable@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> fs/exec.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/fs/exec.c b/fs/exec.c >> index 72934df68471..8079ca70cfda 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> >> if (write) { >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; >> + unsigned long ptr_size; >> struct rlimit *rlim; >> >> + /* >> + * Since the stack will hold pointers to the strings, we >> + * must account for them as well. >> + */ >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); >> + if (ptr_size > ULONG_MAX - size) >> + goto fail; >> + size += ptr_size; >> + >> acct_arg_size(bprm, size / PAGE_SIZE); > > Doesn't this over account? I mean this gets called for partial arguments > as they fit into a page so a single argument can get into this function > multiple times AFAIU. I also do not understand why would you want to > account bprm->argc + bprm->envc pointers for each argument. Based on what I could understand in acct_arg_size(), this is called repeatedly with with the "current" size (it handles the difference between prior calls, see calls like acct_arg_size(bprm, 0)). The size calculation is the entire vma while each arg page is built, so each time we get here it's calculating how far it is currently (rather than each call being just the newly added size from the arg page). As a result, we need to always add the entire size of the pointers, so that on the last call to get_arg_page() we'll actually have the entire correct size. -Kees > >> >> /* >> @@ -239,13 +249,15 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, >> * to work from. >> */ >> rlim = current->signal->rlim; >> - if (size > ACCESS_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) { >> - put_page(page); >> - return NULL; >> - } >> + if (size > READ_ONCE(rlim[RLIMIT_STACK].rlim_cur) / 4) >> + goto fail; >> } >> >> return page; >> + >> +fail: >> + put_page(page); >> + return NULL; >> } >> >> static void put_arg_page(struct page *page) >> -- >> 2.7.4 >> >> >> -- >> Kees Cook >> Pixel Security >> >> -- >> To unsubscribe, send a message with 'unsubscribe linux-mm' in >> the body to majordomo@kvack.org. For more info on Linux MM, >> see: http://www.linux-mm.org/ . >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Michal Hocko > SUSE Labs -- Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers 2017-06-23 14:05 ` Kees Cook (?) @ 2017-06-23 14:18 ` Michal Hocko -1 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 14:18 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko <mhocko@suse.com> Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* [kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 14:18 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 14:18 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko <mhocko@suse.com> Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] exec: Account for argv/envp pointers @ 2017-06-23 14:18 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2017-06-23 14:18 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Qualys Security Advisory, Linux-MM, linux-fsdevel, LKML, kernel-hardening On Fri 23-06-17 07:05:37, Kees Cook wrote: > On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <mhocko@kernel.org> wrote: [...] > >> --- a/fs/exec.c > >> +++ b/fs/exec.c > >> @@ -220,8 +220,18 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, > >> > >> if (write) { > >> unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start; > >> + unsigned long ptr_size; > >> struct rlimit *rlim; > >> > >> + /* > >> + * Since the stack will hold pointers to the strings, we > >> + * must account for them as well. > >> + */ > >> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *); > >> + if (ptr_size > ULONG_MAX - size) > >> + goto fail; > >> + size += ptr_size; > >> + > >> acct_arg_size(bprm, size / PAGE_SIZE); > > > > Doesn't this over account? I mean this gets called for partial arguments > > as they fit into a page so a single argument can get into this function > > multiple times AFAIU. I also do not understand why would you want to > > account bprm->argc + bprm->envc pointers for each argument. > > Based on what I could understand in acct_arg_size(), this is called > repeatedly with with the "current" size (it handles the difference > between prior calls, see calls like acct_arg_size(bprm, 0)). > > The size calculation is the entire vma while each arg page is built, > so each time we get here it's calculating how far it is currently > (rather than each call being just the newly added size from the arg > page). As a result, we need to always add the entire size of the > pointers, so that on the last call to get_arg_page() we'll actually > have the entire correct size. Ohh, I forgot about this tricky part. The code just looks confusing becauser we are mixing 2 things together here. This deserves a comment I guess. Other than that feel free to add Acked-by: Michal Hocko <mhocko@suse.com> Thanks! -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-06-23 14:18 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-22 0:17 [PATCH] exec: Account for argv/envp pointers Kees Cook 2017-06-22 0:17 ` [kernel-hardening] " Kees Cook 2017-06-22 0:17 ` Kees Cook 2017-06-22 1:39 ` [kernel-hardening] " Rik van Riel 2017-06-22 1:39 ` Rik van Riel 2017-06-23 13:59 ` Michal Hocko 2017-06-23 13:59 ` [kernel-hardening] " Michal Hocko 2017-06-23 13:59 ` Michal Hocko 2017-06-23 14:05 ` Kees Cook 2017-06-23 14:05 ` [kernel-hardening] " Kees Cook 2017-06-23 14:05 ` Kees Cook 2017-06-23 14:18 ` Michal Hocko 2017-06-23 14:18 ` [kernel-hardening] " Michal Hocko 2017-06-23 14:18 ` Michal Hocko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.