[PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Alexey Dobriyan]

/proc/*/cmdline is not different from /proc/*/environ as it accesses
target task's memory (and can access the very same region of memory)
but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.

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

 fs/proc/base.c |   67 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -208,11 +208,34 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
+static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
+{
+	struct mm_struct *mm = proc_mem_open(inode, mode);
+
+	if (IS_ERR(mm))
+		return PTR_ERR(mm);
+
+	file->private_data = mm;
+	return 0;
+}
+
+static int proc_pid_cmdline_open(struct inode *inode, struct file *file)
+{
+	return __mem_open(inode, file, PTRACE_MODE_READ);
+}
+
+static int mem_release(struct inode *inode, struct file *file)
+{
+	struct mm_struct *mm = file->private_data;
+	if (mm)
+		mmdrop(mm);
+	return 0;
+}
+
 static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				     size_t _count, loff_t *pos)
 {
-	struct task_struct *tsk;
-	struct mm_struct *mm;
+	struct mm_struct *mm = file->private_data;
 	char *page;
 	unsigned long count = _count;
 	unsigned long arg_start, arg_end, env_start, env_end;
@@ -223,18 +246,11 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 
 	BUG_ON(*pos < 0);
 
-	tsk = get_proc_task(file_inode(file));
-	if (!tsk)
-		return -ESRCH;
-	mm = get_task_mm(tsk);
-	put_task_struct(tsk);
-	if (!mm)
-		return 0;
 	/* Check if process spawned far enough to have cmdline. */
-	if (!mm->env_end) {
-		rv = 0;
-		goto out_mmput;
-	}
+	if (!mm || !mm->env_end)
+		return 0;
+	if (!mmget_not_zero(mm))
+		return 0;
 
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page) {
@@ -376,8 +392,10 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 }
 
 static const struct file_operations proc_pid_cmdline_ops = {
-	.read	= proc_pid_cmdline_read,
-	.llseek	= generic_file_llseek,
+	.open		= proc_pid_cmdline_open,
+	.read		= proc_pid_cmdline_read,
+	.llseek		= generic_file_llseek,
+	.release	= mem_release,
 };
 
 #ifdef CONFIG_KALLSYMS
@@ -786,17 +804,6 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 	return mm;
 }
 
-static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
-{
-	struct mm_struct *mm = proc_mem_open(inode, mode);
-
-	if (IS_ERR(mm))
-		return PTR_ERR(mm);
-
-	file->private_data = mm;
-	return 0;
-}
-
 static int mem_open(struct inode *inode, struct file *file)
 {
 	int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
@@ -890,14 +897,6 @@ loff_t mem_lseek(struct file *file, loff_t offset, int orig)
 	return file->f_pos;
 }
 
-static int mem_release(struct inode *inode, struct file *file)
-{
-	struct mm_struct *mm = file->private_data;
-	if (mm)
-		mmdrop(mm);
-	return 0;
-}
-
 static const struct file_operations proc_mem_operations = {
 	.llseek		= mem_lseek,
 	.read		= mem_read,

[PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline

[Alexey Dobriyan]

access_remote_vm() doesn't return negative errors, it returns number of
bytes read/written (0 if error occurs). This allows to delete some
comparisons which never trigger.

Reuse "nr_read" variable while I'm at it.

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

 fs/proc/base.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -280,9 +280,10 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	 * Inherently racy -- command line shares address space
 	 * with code and data.
 	 */
-	rv = access_remote_vm(mm, arg_end - 1, &c, 1, 0);
-	if (rv <= 0)
+	if (access_remote_vm(mm, arg_end - 1, &c, 1, 0) != 1) {
+		rv = 0;
 		goto out_free_page;
+	}
 
 	rv = 0;
 
@@ -294,14 +295,11 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 		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, 0);
-			if (nr_read < 0)
-				rv = nr_read;
-			if (nr_read <= 0)
+			unsigned int nr_read;
+
+			nr_read = min3(count, len, PAGE_SIZE);
+			nr_read = access_remote_vm(mm, p, page, nr_read, 0);
+			if (nr_read == 0)
 				goto out_free_page;
 
 			if (copy_to_user(buf, page, nr_read)) {
@@ -339,15 +337,12 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 			p = cmdline[i].p + pos1;
 			len = cmdline[i].len - pos1;
 			while (count > 0 && len > 0) {
-				unsigned int _count, l;
-				int nr_read;
+				unsigned int nr_read, l;
 				bool final;
 
-				_count = min3(count, len, PAGE_SIZE);
-				nr_read = access_remote_vm(mm, p, page, _count, 0);
-				if (nr_read < 0)
-					rv = nr_read;
-				if (nr_read <= 0)
+				nr_read = min3(count, len, PAGE_SIZE);
+				nr_read = access_remote_vm(mm, p, page, nr_read, 0);
+				if (nr_read == 0)
 					goto out_free_page;
 
 				/*

[PATCH 3/5] proc: somewhat simpler code for /proc/*/cmdline

[Alexey Dobriyan]

"final" variable is OK but we can get away with less lines.

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

 fs/proc/base.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -337,8 +337,7 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 			p = cmdline[i].p + pos1;
 			len = cmdline[i].len - pos1;
 			while (count > 0 && len > 0) {
-				unsigned int nr_read, l;
-				bool final;
+				unsigned int nr_read, nr_write;
 
 				nr_read = min3(count, len, PAGE_SIZE);
 				nr_read = access_remote_vm(mm, p, page, nr_read, 0);
@@ -349,25 +348,20 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				 * 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;
-				}
+				nr_write = strnlen(page, nr_read);
 
-				if (copy_to_user(buf, page, nr_read)) {
+				if (copy_to_user(buf, page, nr_write)) {
 					rv = -EFAULT;
 					goto out_free_page;
 				}
 
-				p	+= nr_read;
-				len	-= nr_read;
-				buf	+= nr_read;
-				count	-= nr_read;
-				rv	+= nr_read;
+				p	+= nr_write;
+				len	-= nr_write;
+				buf	+= nr_write;
+				count	-= nr_write;
+				rv	+= nr_write;
 
-				if (final)
+				if (nr_write < nr_read)
 					goto out_free_page;
 			}
 

Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Andy Shevchenko]

On Wed, Feb 21, 2018 at 9:23 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> /proc/*/cmdline is not different from /proc/*/environ as it accesses
> target task's memory (and can access the very same region of memory)
> but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.

> +static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
> +{
> +       struct mm_struct *mm = proc_mem_open(inode, mode);
> +
> +       if (IS_ERR(mm))
> +               return PTR_ERR(mm);

So, is it possible to have it NULL?..

> +
> +       file->private_data = mm;
> +       return 0;
> +}

> +static int mem_release(struct inode *inode, struct file *file)
> +{
> +       struct mm_struct *mm = file->private_data;

> +       if (mm)

...or I don't get this check.

> +               mmdrop(mm);
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/5] proc: simpler iterations for /proc/*/cmdline

[Alexey Dobriyan]

"rv" variable is used both as a counter of bytes transferred and
an error value holder but it can be reduced solely to error values
if original start of userspace buffer is stashed and used at the very
end.

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

 fs/proc/base.c |   36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -241,8 +241,9 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	unsigned long arg_start, arg_end, env_start, env_end;
 	unsigned long len1, len2, len;
 	unsigned long p;
+	char __user *buf0 = buf;
 	char c;
-	ssize_t rv;
+	int rv;
 
 	BUG_ON(*pos < 0);
 
@@ -272,25 +273,20 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	len2 = env_end - env_start;
 
 	/* Empty ARGV. */
-	if (len1 == 0) {
-		rv = 0;
-		goto out_free_page;
-	}
+	if (len1 == 0)
+		goto end;
+
 	/*
 	 * Inherently racy -- command line shares address space
 	 * with code and data.
 	 */
-	if (access_remote_vm(mm, arg_end - 1, &c, 1, 0) != 1) {
-		rv = 0;
-		goto out_free_page;
-	}
-
-	rv = 0;
+	if (access_remote_vm(mm, arg_end - 1, &c, 1, 0) != 1)
+		goto end;
 
 	if (c == '\0') {
 		/* Command line (set of strings) occupies whole ARGV. */
 		if (len1 <= *pos)
-			goto out_free_page;
+			goto end;
 
 		p = arg_start + *pos;
 		len = len1 - *pos;
@@ -300,7 +296,7 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 			nr_read = min3(count, len, PAGE_SIZE);
 			nr_read = access_remote_vm(mm, p, page, nr_read, 0);
 			if (nr_read == 0)
-				goto out_free_page;
+				goto end;
 
 			if (copy_to_user(buf, page, nr_read)) {
 				rv = -EFAULT;
@@ -311,7 +307,6 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 			len	-= nr_read;
 			buf	+= nr_read;
 			count	-= nr_read;
-			rv	+= nr_read;
 		}
 	} else {
 		/*
@@ -342,7 +337,7 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				nr_read = min3(count, len, PAGE_SIZE);
 				nr_read = access_remote_vm(mm, p, page, nr_read, 0);
 				if (nr_read == 0)
-					goto out_free_page;
+					goto end;
 
 				/*
 				 * Command line can be shorter than whole ARGV
@@ -359,10 +354,9 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				len	-= nr_write;
 				buf	+= nr_write;
 				count	-= nr_write;
-				rv	+= nr_write;
 
 				if (nr_write < nr_read)
-					goto out_free_page;
+					goto end;
 			}
 
 			/* Only first chunk can be read partially. */
@@ -371,12 +365,16 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 		}
 	}
 
+end:
+	free_page((unsigned long)page);
+	mmput(mm);
+	*pos += buf - buf0;
+	return buf - buf0;
+
 out_free_page:
 	free_page((unsigned long)page);
 out_mmput:
 	mmput(mm);
-	if (rv > 0)
-		*pos += rv;
 	return rv;
 }
 

[PATCH 5/5] proc: deduplicate /proc/*/cmdline implementation

[Alexey Dobriyan]

Code can be sonsolidated if a dummy region of 0 length is used in normal
case of \0-separated command line:

1) [arg_start, arg_end) + [dummy len=0]
2) [arg_start, arg_end) + [env_start, env_end)

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

 fs/proc/base.c |   53 ++++++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -239,9 +239,12 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	char *page;
 	unsigned long count = _count;
 	unsigned long arg_start, arg_end, env_start, env_end;
-	unsigned long len1, len2, len;
-	unsigned long p;
+	unsigned long len1, len2;
 	char __user *buf0 = buf;
+	struct {
+		unsigned long p;
+		unsigned long len;
+	} cmdline[2];
 	char c;
 	int rv;
 
@@ -283,43 +286,21 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 	if (access_remote_vm(mm, arg_end - 1, &c, 1, 0) != 1)
 		goto end;
 
+	cmdline[0].p = arg_start;
+	cmdline[0].len = len1;
 	if (c == '\0') {
 		/* Command line (set of strings) occupies whole ARGV. */
-		if (len1 <= *pos)
-			goto end;
-
-		p = arg_start + *pos;
-		len = len1 - *pos;
-		while (count > 0 && len > 0) {
-			unsigned int nr_read;
-
-			nr_read = min3(count, len, PAGE_SIZE);
-			nr_read = access_remote_vm(mm, p, page, nr_read, 0);
-			if (nr_read == 0)
-				goto end;
-
-			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;
-		}
+		cmdline[1].len = 0;
 	} else {
 		/*
 		 * Command line (1 string) occupies ARGV and
 		 * extends into ENVP.
 		 */
-		struct {
-			unsigned long p;
-			unsigned long len;
-		} cmdline[2] = {
-			{ .p = arg_start, .len = len1 },
-			{ .p = env_start, .len = len2 },
-		};
+		cmdline[1].p = env_start;
+		cmdline[1].len = len2;
+	}
+
+	{
 		loff_t pos1 = *pos;
 		unsigned int i;
 
@@ -329,6 +310,9 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 			i++;
 		}
 		while (i < 2) {
+			unsigned long p;
+			unsigned long len;
+
 			p = cmdline[i].p + pos1;
 			len = cmdline[i].len - pos1;
 			while (count > 0 && len > 0) {
@@ -343,7 +327,10 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				 * Command line can be shorter than whole ARGV
 				 * even if last "marker" byte says it is not.
 				 */
-				nr_write = strnlen(page, nr_read);
+				if (c == '\0')
+					nr_write = nr_read;
+				else
+					nr_write = strnlen(page, nr_read);
 
 				if (copy_to_user(buf, page, nr_write)) {
 					rv = -EFAULT;

Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Alexey Dobriyan]

On Wed, Feb 21, 2018 at 09:28:41PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2018 at 9:23 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > /proc/*/cmdline is not different from /proc/*/environ as it accesses
> > target task's memory (and can access the very same region of memory)
> > but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.
> 
> > +static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
> > +{
> > +       struct mm_struct *mm = proc_mem_open(inode, mode);
> > +
> > +       if (IS_ERR(mm))
> > +               return PTR_ERR(mm);
> 
> So, is it possible to have it NULL?..

I haven't looked closely, but if kernel thread is accesses then yes.

Regardless, patch only moves function so that code compiles, untangling
this little mess is separate adventure.

> > +static int mem_release(struct inode *inode, struct file *file)
> > +{
> > +       struct mm_struct *mm = file->private_data;
> 
> > +       if (mm)
> 
> ...or I don't get this check.
> 
> > +               mmdrop(mm);
> > +       return 0;
> > +}

It should trigger if kernel thread is accessed.

Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Andrew Morton]

On Wed, 21 Feb 2018 22:23:39 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> /proc/*/cmdline is not different from /proc/*/environ as it accesses
> target task's memory (and can access the very same region of memory)
> but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.
> 

I'd really like to see more thoughtful changelogging, please.

Why are we doing this?  What is the advantage?

Doesn't this mean that code which could previously read
/proc/pid/cmdline may no longer be able to do so?  Can't this break
userspace?  Discuss.  Lots!

[PATCH v2 1/5] proc: make /proc/*/cmdline go through LSM

[Alexey Dobriyan]

/proc/*/mem and /proc/*/environ use common code to check if open is
allowed. Essentially they all access target task memory.

/proc/*/cmdline should be subject to the same checks as it does
what /proc/*/environ does.

->security_ptrace_access_check LSM hook will be called allowing LSM
to interfere.

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

 fs/proc/base.c |   67 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -208,11 +208,34 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
+static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
+{
+	struct mm_struct *mm = proc_mem_open(inode, mode);
+
+	if (IS_ERR(mm))
+		return PTR_ERR(mm);
+
+	file->private_data = mm;
+	return 0;
+}
+
+static int proc_pid_cmdline_open(struct inode *inode, struct file *file)
+{
+	return __mem_open(inode, file, PTRACE_MODE_READ);
+}
+
+static int mem_release(struct inode *inode, struct file *file)
+{
+	struct mm_struct *mm = file->private_data;
+	if (mm)
+		mmdrop(mm);
+	return 0;
+}
+
 static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 				     size_t _count, loff_t *pos)
 {
-	struct task_struct *tsk;
-	struct mm_struct *mm;
+	struct mm_struct *mm = file->private_data;
 	char *page;
 	unsigned long count = _count;
 	unsigned long arg_start, arg_end, env_start, env_end;
@@ -223,18 +246,11 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 
 	BUG_ON(*pos < 0);
 
-	tsk = get_proc_task(file_inode(file));
-	if (!tsk)
-		return -ESRCH;
-	mm = get_task_mm(tsk);
-	put_task_struct(tsk);
-	if (!mm)
-		return 0;
 	/* Check if process spawned far enough to have cmdline. */
-	if (!mm->env_end) {
-		rv = 0;
-		goto out_mmput;
-	}
+	if (!mm || !mm->env_end)
+		return 0;
+	if (!mmget_not_zero(mm))
+		return 0;
 
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page) {
@@ -376,8 +392,10 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
 }
 
 static const struct file_operations proc_pid_cmdline_ops = {
-	.read	= proc_pid_cmdline_read,
-	.llseek	= generic_file_llseek,
+	.open		= proc_pid_cmdline_open,
+	.read		= proc_pid_cmdline_read,
+	.llseek		= generic_file_llseek,
+	.release	= mem_release,
 };
 
 #ifdef CONFIG_KALLSYMS
@@ -786,17 +804,6 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
 	return mm;
 }
 
-static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
-{
-	struct mm_struct *mm = proc_mem_open(inode, mode);
-
-	if (IS_ERR(mm))
-		return PTR_ERR(mm);
-
-	file->private_data = mm;
-	return 0;
-}
-
 static int mem_open(struct inode *inode, struct file *file)
 {
 	int ret = __mem_open(inode, file, PTRACE_MODE_ATTACH);
@@ -890,14 +897,6 @@ loff_t mem_lseek(struct file *file, loff_t offset, int orig)
 	return file->f_pos;
 }
 
-static int mem_release(struct inode *inode, struct file *file)
-{
-	struct mm_struct *mm = file->private_data;
-	if (mm)
-		mmdrop(mm);
-	return 0;
-}
-
 static const struct file_operations proc_mem_operations = {
 	.llseek		= mem_lseek,
 	.read		= mem_read,

Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Andrew Morton]

On Wed, 21 Feb 2018 22:23:39 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> /proc/*/cmdline is not different from /proc/*/environ as it accesses
> target task's memory (and can access the very same region of memory)
> but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.

This change can cause existing code to break, no?

I'd like to see, in the changelog, a full explanation of why this won't
break any existing setup?

Re: [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline

[Andrew Morton]

On Wed, 21 Feb 2018 22:30:09 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> "rv" variable is used both as a counter of bytes transferred and
> an error value holder but it can be reduced solely to error values
> if original start of userspace buffer is stashed and used at the very
> end.
> 
> ...
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
>
> ...
>
> @@ -371,12 +365,16 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
>  		}
>  	}
>  
> +end:
> +	free_page((unsigned long)page);
> +	mmput(mm);
> +	*pos += buf - buf0;
> +	return buf - buf0;
> +
>  out_free_page:
>  	free_page((unsigned long)page);
>  out_mmput:
>  	mmput(mm);
> -	if (rv > 0)
> -		*pos += rv;
>  	return rv;
>  }

(scratches head).  Why not do this?

--- a/fs/proc/base.c~proc-simpler-iterations-for-proc-cmdline-fix
+++ a/fs/proc/base.c
@@ -363,11 +363,8 @@ static ssize_t proc_pid_cmdline_read(str
 	}
 
 end:
-	free_page((unsigned long)page);
-	mmput(mm);
 	*pos += buf - buf0;
-	return buf - buf0;
-
+	rv = buf - buf0;
 out_free_page:
 	free_page((unsigned long)page);
 out_mmput:

(and maybe rv should be ssize_t)

Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM

[Alexey Dobriyan]

On Thu, Apr 19, 2018 at 05:02:17PM -0700, Andrew Morton wrote:
> On Wed, 21 Feb 2018 22:23:39 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > /proc/*/cmdline is not different from /proc/*/environ as it accesses
> > target task's memory (and can access the very same region of memory)
> > but it doesn't go through ptrace_may_access() and thus doesn't go through LSM.
> 
> This change can cause existing code to break, no?
> 
> I'd like to see, in the changelog, a full explanation of why this won't
> break any existing setup?

It can. In fact, I'm not sure about this patch anymore.
Original /proc/*/cmdline hook did get_cmdline() which is basically GUP.
It is just nobody said anything when /proc/*/cmdline got rewritten and
security folks aren't bitching about it:

	$ cat /proc/1/cmdline
	init [3]

I'll resend the rest of cmdline changes if they gets broken.

Re: [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline

[Alexey Dobriyan]

On Thu, Apr 19, 2018 at 05:15:20PM -0700, Andrew Morton wrote:
> On Wed, 21 Feb 2018 22:30:09 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > "rv" variable is used both as a counter of bytes transferred and
> > an error value holder but it can be reduced solely to error values
> > if original start of userspace buffer is stashed and used at the very
> > end.
> > 
> > ...
> >
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> >
> > ...
> >
> > @@ -371,12 +365,16 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf,
> >  		}
> >  	}
> >  
> > +end:
> > +	free_page((unsigned long)page);
> > +	mmput(mm);
> > +	*pos += buf - buf0;
> > +	return buf - buf0;
> > +
> >  out_free_page:
> >  	free_page((unsigned long)page);
> >  out_mmput:
> >  	mmput(mm);
> > -	if (rv > 0)
> > -		*pos += rv;
> >  	return rv;
> >  }
> 
> (scratches head).  Why not do this?
> 
> --- a/fs/proc/base.c~proc-simpler-iterations-for-proc-cmdline-fix
> +++ a/fs/proc/base.c
> @@ -363,11 +363,8 @@ static ssize_t proc_pid_cmdline_read(str
>  	}
>  
>  end:
> -	free_page((unsigned long)page);
> -	mmput(mm);
>  	*pos += buf - buf0;
> -	return buf - buf0;
> -
> +	rv = buf - buf0;
>  out_free_page:
>  	free_page((unsigned long)page);
>  out_mmput:
> 
> (and maybe rv should be ssize_t)

It should as cmdline area can be set to arbitrary values.

I hate ssize_t and it's bloat.

	$ ./scripts/bloat-o-meter ../vmlinux-000 ../obj/vmlinux
	add/remove: 0/0 grow/shrink: 1/0 up/down: 11/0 (11)
	Function                                     old     new   delta
	proc_pid_cmdline_read                        867     878     +11