[PATCH] exec: Account for argv/envp pointers

[PATCH] exec: Account for argv/envp pointers

[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")
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

[PATCH] exec: Account for argv/envp pointers

[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")
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>

[kernel-hardening] [PATCH] exec: Account for argv/envp pointers

[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")
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

Re: [kernel-hardening] [PATCH] exec: Account for argv/envp pointers

[Rik van Riel]

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>

Re: [kernel-hardening] [PATCH] exec: Account for argv/envp pointers

[Rik van Riel]

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>

Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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

Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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>

[kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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

Re: [PATCH] exec: Account for argv/envp pointers

[Kees Cook]

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

Re: [PATCH] exec: Account for argv/envp pointers

[Kees Cook]

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>

[kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers

[Kees Cook]

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

Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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

Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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>

[kernel-hardening] Re: [PATCH] exec: Account for argv/envp pointers

[Michal Hocko]

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