proc: revert /proc/*/cmdline rewrite
diff mbox series

Message ID 20190713073209.GC23167@avx2
State New
Headers show
Series
  • proc: revert /proc/*/cmdline rewrite
Related show

Commit Message

Alexey Dobriyan July 13, 2019, 7:32 a.m. UTC
/proc/*/cmdline continues to cause problems:

	https://lkml.org/lkml/2019/4/5/825
	Subject get_mm_cmdline and userspace (Perl) changing argv0

	https://marc.info/?l=linux-kernel&m=156294831308786&w=4
	[PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

This patch reverts implementation to the last known good state.
Revert

	commit f5b65348fd77839b50e79bc0a5e536832ea52d8d
	proc: fix missing final NUL in get_mm_cmdline() rewrite

	commit 5ab8271899658042fabc5ae7e6a99066a210bc0e
	fs/proc: simplify and clarify get_mm_cmdline() function

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

	Cc lists

 fs/proc/base.c |  198 +++++++++++++++++++++++++++++++++------------------------
 1 file changed, 118 insertions(+), 80 deletions(-)

Comments

Alexey Izbyshev July 13, 2019, 2 p.m. UTC | #1
On 7/13/19 10:32 AM, Alexey Dobriyan wrote:
> /proc/*/cmdline continues to cause problems:
> 
> 	https://lkml.org/lkml/2019/4/5/825
> 	Subject get_mm_cmdline and userspace (Perl) changing argv0
> 
> 	https://marc.info/?l=linux-kernel&m=156294831308786&w=4
> 	[PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()
> 
> This patch reverts implementation to the last known good state.

I confirm that this revert fixes both issues above on my system (x86_64).

Alexey
Eric W. Biederman July 14, 2019, 4:16 a.m. UTC | #2
Alexey Dobriyan <adobriyan@gmail.com> writes:

> /proc/*/cmdline continues to cause problems:
>
> 	https://lkml.org/lkml/2019/4/5/825
> 	Subject get_mm_cmdline and userspace (Perl) changing argv0
>
> 	https://marc.info/?l=linux-kernel&m=156294831308786&w=4
> 	[PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()
>
> This patch reverts implementation to the last known good state.
> Revert
>
> 	commit f5b65348fd77839b50e79bc0a5e536832ea52d8d
> 	proc: fix missing final NUL in get_mm_cmdline() rewrite
>
> 	commit 5ab8271899658042fabc5ae7e6a99066a210bc0e
> 	fs/proc: simplify and clarify get_mm_cmdline() function
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Given that this fixes a regression and a bug.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

> ---
>
> 	Cc lists
>
>  fs/proc/base.c |  198 +++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 118 insertions(+), 80 deletions(-)
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -210,16 +210,24 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
>  }
>  
>  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> -			      size_t count, loff_t *ppos)
> +			      size_t _count, loff_t *pos)
>  {
> +	unsigned long count = _count;
>  	unsigned long arg_start, arg_end, env_start, env_end;
> -	unsigned long pos, len;
> +	unsigned long len1, len2, len;
> +	unsigned long p;
>  	char *page;
> +	ssize_t rv;
> +	char c;
>  
>  	/* Check if process spawned far enough to have cmdline. */
>  	if (!mm->env_end)
>  		return 0;
>  
> +	page = (char *)__get_free_page(GFP_KERNEL);
> +	if (!page)
> +		return -ENOMEM;
> +
>  	spin_lock(&mm->arg_lock);
>  	arg_start = mm->arg_start;
>  	arg_end = mm->arg_end;
> @@ -227,96 +235,126 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  	env_end = mm->env_end;
>  	spin_unlock(&mm->arg_lock);
>  
> -	if (arg_start >= arg_end)
> -		return 0;
> +	BUG_ON(arg_start > arg_end);
> +	BUG_ON(env_start > env_end);
> +
> +	len1 = arg_end - arg_start;
> +	len2 = env_end - env_start;
>  
> +	/* Empty ARGV. */
> +	if (len1 == 0) {
> +		rv = 0;
> +		goto out_free_page;
> +	}
>  	/*
> -	 * We have traditionally allowed the user to re-write
> -	 * the argument strings and overflow the end result
> -	 * into the environment section. But only do that if
> -	 * the environment area is contiguous to the arguments.
> +	 * Inherently racy -- command line shares address space
> +	 * with code and data.
>  	 */
> -	if (env_start != arg_end || env_start >= env_end)
> -		env_start = env_end = arg_end;
> -
> -	/* .. and limit it to a maximum of one page of slop */
> -	if (env_end >= arg_end + PAGE_SIZE)
> -		env_end = arg_end + PAGE_SIZE - 1;
> -
> -	/* We're not going to care if "*ppos" has high bits set */
> -	pos = arg_start + *ppos;
> -
> -	/* .. but we do check the result is in the proper range */
> -	if (pos < arg_start || pos >= env_end)
> -		return 0;
> -
> -	/* .. and we never go past env_end */
> -	if (env_end - pos < count)
> -		count = env_end - pos;
> -
> -	page = (char *)__get_free_page(GFP_KERNEL);
> -	if (!page)
> -		return -ENOMEM;
> -
> -	len = 0;
> -	while (count) {
> -		int got;
> -		size_t size = min_t(size_t, PAGE_SIZE, count);
> -		long offset;
> +	rv = access_remote_vm(mm, arg_end - 1, &c, 1, FOLL_ANON);
> +	if (rv <= 0)
> +		goto out_free_page;
> +
> +	rv = 0;
> +
> +	if (c == '\0') {
> +		/* Command line (set of strings) occupies whole ARGV. */
> +		if (len1 <= *pos)
> +			goto out_free_page;
> +
> +		p = arg_start + *pos;
> +		len = len1 - *pos;
> +		while (count > 0 && len > 0) {
> +			unsigned int _count;
> +			int nr_read;
> +
> +			_count = min3(count, len, PAGE_SIZE);
> +			nr_read = access_remote_vm(mm, p, page, _count, FOLL_ANON);
> +			if (nr_read < 0)
> +				rv = nr_read;
> +			if (nr_read <= 0)
> +				goto out_free_page;
> +
> +			if (copy_to_user(buf, page, nr_read)) {
> +				rv = -EFAULT;
> +				goto out_free_page;
> +			}
>  
> +			p	+= nr_read;
> +			len	-= nr_read;
> +			buf	+= nr_read;
> +			count	-= nr_read;
> +			rv	+= nr_read;
> +		}
> +	} else {
>  		/*
> -		 * Are we already starting past the official end?
> -		 * We always include the last byte that is *supposed*
> -		 * to be NUL
> +		 * Command line (1 string) occupies ARGV and
> +		 * extends into ENVP.
>  		 */
> -		offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> -
> -		got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);
> -		if (got <= offset)
> -			break;
> -		got -= offset;
> -
> -		/* Don't walk past a NUL character once you hit arg_end */
> -		if (pos + got >= arg_end) {
> -			int n = 0;
> -
> -			/*
> -			 * If we started before 'arg_end' but ended up
> -			 * at or after it, we start the NUL character
> -			 * check at arg_end-1 (where we expect the normal
> -			 * EOF to be).
> -			 *
> -			 * NOTE! This is smaller than 'got', because
> -			 * pos + got >= arg_end
> -			 */
> -			if (pos < arg_end)
> -				n = arg_end - pos - 1;
> -
> -			/* Cut off at first NUL after 'n' */
> -			got = n + strnlen(page+n, offset+got-n);
> -			if (got < offset)
> -				break;
> -			got -= offset;
> -
> -			/* Include the NUL if it existed */
> -			if (got < size)
> -				got++;
> +		struct {
> +			unsigned long p;
> +			unsigned long len;
> +		} cmdline[2] = {
> +			{ .p = arg_start, .len = len1 },
> +			{ .p = env_start, .len = len2 },
> +		};
> +		loff_t pos1 = *pos;
> +		unsigned int i;
> +
> +		i = 0;
> +		while (i < 2 && pos1 >= cmdline[i].len) {
> +			pos1 -= cmdline[i].len;
> +			i++;
>  		}
> +		while (i < 2) {
> +			p = cmdline[i].p + pos1;
> +			len = cmdline[i].len - pos1;
> +			while (count > 0 && len > 0) {
> +				unsigned int _count, l;
> +				int nr_read;
> +				bool final;
> +
> +				_count = min3(count, len, PAGE_SIZE);
> +				nr_read = access_remote_vm(mm, p, page, _count, FOLL_ANON);
> +				if (nr_read < 0)
> +					rv = nr_read;
> +				if (nr_read <= 0)
> +					goto out_free_page;
> +
> +				/*
> +				 * Command line can be shorter than whole ARGV
> +				 * even if last "marker" byte says it is not.
> +				 */
> +				final = false;
> +				l = strnlen(page, nr_read);
> +				if (l < nr_read) {
> +					nr_read = l;
> +					final = true;
> +				}
> +
> +				if (copy_to_user(buf, page, nr_read)) {
> +					rv = -EFAULT;
> +					goto out_free_page;
> +				}
> +
> +				p	+= nr_read;
> +				len	-= nr_read;
> +				buf	+= nr_read;
> +				count	-= nr_read;
> +				rv	+= nr_read;
> +
> +				if (final)
> +					goto out_free_page;
> +			}
>  
> -		got -= copy_to_user(buf, page+offset, got);
> -		if (unlikely(!got)) {
> -			if (!len)
> -				len = -EFAULT;
> -			break;
> +			/* Only first chunk can be read partially. */
> +			pos1 = 0;
> +			i++;
>  		}
> -		pos += got;
> -		buf += got;
> -		len += got;
> -		count -= got;
>  	}
>  
> +out_free_page:
>  	free_page((unsigned long)page);
> -	return len;
> +	return rv;
>  }
>  
>  static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,
Linus Torvalds July 14, 2019, 5:02 a.m. UTC | #3
[ Apparently this thread wasn't on the lists, and I didn't even
notice. So re-sending the patches ]

On Sat, Jul 13, 2019 at 9:16 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Given that this fixes a regression and a bug.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

I'd much rather start from scratch. Like the attached.

Alexey Izbyshev has a third patch that then limits the setproctitle()
case to only allow looking into the env[] area, which looks
reasonable.

           Linus

Patch
diff mbox series

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -210,16 +210,24 @@  static int proc_root_link(struct dentry *dentry, struct path *path)
 }
 
 static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
-			      size_t count, loff_t *ppos)
+			      size_t _count, loff_t *pos)
 {
+	unsigned long count = _count;
 	unsigned long arg_start, arg_end, env_start, env_end;
-	unsigned long pos, len;
+	unsigned long len1, len2, len;
+	unsigned long p;
 	char *page;
+	ssize_t rv;
+	char c;
 
 	/* Check if process spawned far enough to have cmdline. */
 	if (!mm->env_end)
 		return 0;
 
+	page = (char *)__get_free_page(GFP_KERNEL);
+	if (!page)
+		return -ENOMEM;
+
 	spin_lock(&mm->arg_lock);
 	arg_start = mm->arg_start;
 	arg_end = mm->arg_end;
@@ -227,96 +235,126 @@  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 	env_end = mm->env_end;
 	spin_unlock(&mm->arg_lock);
 
-	if (arg_start >= arg_end)
-		return 0;
+	BUG_ON(arg_start > arg_end);
+	BUG_ON(env_start > env_end);
+
+	len1 = arg_end - arg_start;
+	len2 = env_end - env_start;
 
+	/* Empty ARGV. */
+	if (len1 == 0) {
+		rv = 0;
+		goto out_free_page;
+	}
 	/*
-	 * We have traditionally allowed the user to re-write
-	 * the argument strings and overflow the end result
-	 * into the environment section. But only do that if
-	 * the environment area is contiguous to the arguments.
+	 * Inherently racy -- command line shares address space
+	 * with code and data.
 	 */
-	if (env_start != arg_end || env_start >= env_end)
-		env_start = env_end = arg_end;
-
-	/* .. and limit it to a maximum of one page of slop */
-	if (env_end >= arg_end + PAGE_SIZE)
-		env_end = arg_end + PAGE_SIZE - 1;
-
-	/* We're not going to care if "*ppos" has high bits set */
-	pos = arg_start + *ppos;
-
-	/* .. but we do check the result is in the proper range */
-	if (pos < arg_start || pos >= env_end)
-		return 0;
-
-	/* .. and we never go past env_end */
-	if (env_end - pos < count)
-		count = env_end - pos;
-
-	page = (char *)__get_free_page(GFP_KERNEL);
-	if (!page)
-		return -ENOMEM;
-
-	len = 0;
-	while (count) {
-		int got;
-		size_t size = min_t(size_t, PAGE_SIZE, count);
-		long offset;
+	rv = access_remote_vm(mm, arg_end - 1, &c, 1, FOLL_ANON);
+	if (rv <= 0)
+		goto out_free_page;
+
+	rv = 0;
+
+	if (c == '\0') {
+		/* Command line (set of strings) occupies whole ARGV. */
+		if (len1 <= *pos)
+			goto out_free_page;
+
+		p = arg_start + *pos;
+		len = len1 - *pos;
+		while (count > 0 && len > 0) {
+			unsigned int _count;
+			int nr_read;
+
+			_count = min3(count, len, PAGE_SIZE);
+			nr_read = access_remote_vm(mm, p, page, _count, FOLL_ANON);
+			if (nr_read < 0)
+				rv = nr_read;
+			if (nr_read <= 0)
+				goto out_free_page;
+
+			if (copy_to_user(buf, page, nr_read)) {
+				rv = -EFAULT;
+				goto out_free_page;
+			}
 
+			p	+= nr_read;
+			len	-= nr_read;
+			buf	+= nr_read;
+			count	-= nr_read;
+			rv	+= nr_read;
+		}
+	} else {
 		/*
-		 * Are we already starting past the official end?
-		 * We always include the last byte that is *supposed*
-		 * to be NUL
+		 * Command line (1 string) occupies ARGV and
+		 * extends into ENVP.
 		 */
-		offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
-
-		got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);
-		if (got <= offset)
-			break;
-		got -= offset;
-
-		/* Don't walk past a NUL character once you hit arg_end */
-		if (pos + got >= arg_end) {
-			int n = 0;
-
-			/*
-			 * If we started before 'arg_end' but ended up
-			 * at or after it, we start the NUL character
-			 * check at arg_end-1 (where we expect the normal
-			 * EOF to be).
-			 *
-			 * NOTE! This is smaller than 'got', because
-			 * pos + got >= arg_end
-			 */
-			if (pos < arg_end)
-				n = arg_end - pos - 1;
-
-			/* Cut off at first NUL after 'n' */
-			got = n + strnlen(page+n, offset+got-n);
-			if (got < offset)
-				break;
-			got -= offset;
-
-			/* Include the NUL if it existed */
-			if (got < size)
-				got++;
+		struct {
+			unsigned long p;
+			unsigned long len;
+		} cmdline[2] = {
+			{ .p = arg_start, .len = len1 },
+			{ .p = env_start, .len = len2 },
+		};
+		loff_t pos1 = *pos;
+		unsigned int i;
+
+		i = 0;
+		while (i < 2 && pos1 >= cmdline[i].len) {
+			pos1 -= cmdline[i].len;
+			i++;
 		}
+		while (i < 2) {
+			p = cmdline[i].p + pos1;
+			len = cmdline[i].len - pos1;
+			while (count > 0 && len > 0) {
+				unsigned int _count, l;
+				int nr_read;
+				bool final;
+
+				_count = min3(count, len, PAGE_SIZE);
+				nr_read = access_remote_vm(mm, p, page, _count, FOLL_ANON);
+				if (nr_read < 0)
+					rv = nr_read;
+				if (nr_read <= 0)
+					goto out_free_page;
+
+				/*
+				 * Command line can be shorter than whole ARGV
+				 * even if last "marker" byte says it is not.
+				 */
+				final = false;
+				l = strnlen(page, nr_read);
+				if (l < nr_read) {
+					nr_read = l;
+					final = true;
+				}
+
+				if (copy_to_user(buf, page, nr_read)) {
+					rv = -EFAULT;
+					goto out_free_page;
+				}
+
+				p	+= nr_read;
+				len	-= nr_read;
+				buf	+= nr_read;
+				count	-= nr_read;
+				rv	+= nr_read;
+
+				if (final)
+					goto out_free_page;
+			}
 
-		got -= copy_to_user(buf, page+offset, got);
-		if (unlikely(!got)) {
-			if (!len)
-				len = -EFAULT;
-			break;
+			/* Only first chunk can be read partially. */
+			pos1 = 0;
+			i++;
 		}
-		pos += got;
-		buf += got;
-		len += got;
-		count -= got;
 	}
 
+out_free_page:
 	free_page((unsigned long)page);
-	return len;
+	return rv;
 }
 
 static ssize_t get_task_cmdline(struct task_struct *tsk, char __user *buf,