exec: Account for argv/envp pointers
Subsystems (3)
PATCH 1/1 exec: Account for argv/envp pointers
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).
--- fs/exec.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
fs/exec.c +16 -4
@@ -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)
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 19 lines · Kees Cook
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).
Quoted 3 lines
Fixes b6a2fea39318 ("mm: variable length argument support")Signed-off-by Kees Cook <[email protected]>
Quoted 30 lines · Kees Cook
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")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)fs/exec.c +1
-0@@ -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;
fs/exec.c static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, · line 235
Quoted 10 lines
fs/exec.c +9
-0+/* +* 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.
Quoted 35 lines
fs/exec.c +6
-4/* @@ -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)Signature · 12 lines
2.7.4 -- Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 31 lines · Michal Hocko
Quoted 30 lines · Kees Cook
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")Signed-off-by Kees Cook <[email protected]>--- fs/exec.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)fs/exec.c +1
-0@@ -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;
Quoted 15 lines
fs/exec.c static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, · line 235
Quoted 10 lines
fs/exec.c +9
-0+/* +* 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->envcpointers 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
Quoted 40 lines
Quoted 31 lines
fs/exec.c +6
-5/* @@ -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 SecurityQuoted 5 lines
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>--
Michal Hocko
SUSE Labs
Signature · 2 lines
Kees Cook Pixel Security
Michal Hocko
Acked-byQuoted 1 line · Kees Cook
On Fri, Jun 23, 2017 at 6:59 AM, Michal Hocko <[email protected]> wrote:
[...]
Quoted 29 lines
Quoted 9 lines
Quoted 9 lines
fs/exec.c +1
-0@@ -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;Quoted 15 lines
fs/exec.c static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, · line 235
Quoted 10 lines
fs/exec.c +9
-0+/* +* 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->envcpointers 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 likeacct_arg_size(bprm, 0)).
Quoted 6 lines
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
Thanks!
Signature · 2 lines
Michal Hocko SUSE Labs