LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
@ 2018-02-21 19:23 Alexey Dobriyan
  2018-02-21 19:26 ` [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline Alexey Dobriyan
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:23 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

/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,

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline
  2018-02-21 19:23 [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Alexey Dobriyan
@ 2018-02-21 19:26 ` Alexey Dobriyan
  2018-02-21 19:27   ` [PATCH 3/5] proc: somewhat simpler code for /proc/*/cmdline Alexey Dobriyan
  2018-02-21 19:28 ` [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Andy Shevchenko
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:26 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

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;
 
 				/*

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/5] proc: somewhat simpler code for /proc/*/cmdline
  2018-02-21 19:26 ` [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline Alexey Dobriyan
@ 2018-02-21 19:27   ` Alexey Dobriyan
  2018-02-21 19:30     ` [PATCH 4/5] proc: simpler iterations " Alexey Dobriyan
  0 siblings, 1 reply; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:27 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

"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;
 			}
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
  2018-02-21 19:23 [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Alexey Dobriyan
  2018-02-21 19:26 ` [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline Alexey Dobriyan
@ 2018-02-21 19:28 ` Andy Shevchenko
  2018-02-21 19:39   ` Alexey Dobriyan
  2018-02-21 20:06 ` Andrew Morton
  2018-04-20  0:02 ` [PATCH " Andrew Morton
  3 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2018-02-21 19:28 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: Andrew Morton, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline
  2018-02-21 19:27   ` [PATCH 3/5] proc: somewhat simpler code for /proc/*/cmdline Alexey Dobriyan
@ 2018-02-21 19:30     ` Alexey Dobriyan
  2018-02-21 19:33       ` [PATCH 5/5] proc: deduplicate /proc/*/cmdline implementation Alexey Dobriyan
  2018-04-20  0:15       ` [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline Andrew Morton
  0 siblings, 2 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:30 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

"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;
 }
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 5/5] proc: deduplicate /proc/*/cmdline implementation
  2018-02-21 19:30     ` [PATCH 4/5] proc: simpler iterations " Alexey Dobriyan
@ 2018-02-21 19:33       ` Alexey Dobriyan
  2018-04-20  0:15       ` [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline Andrew Morton
  1 sibling, 0 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:33 UTC (permalink / raw)
  To: akpm; +Cc: linux-kernel

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;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
  2018-02-21 19:28 ` [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Andy Shevchenko
@ 2018-02-21 19:39   ` Alexey Dobriyan
  0 siblings, 0 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-21 19:39 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Andrew Morton, Linux Kernel Mailing List

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
  2018-02-21 19:23 [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Alexey Dobriyan
  2018-02-21 19:26 ` [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline Alexey Dobriyan
  2018-02-21 19:28 ` [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Andy Shevchenko
@ 2018-02-21 20:06 ` Andrew Morton
  2018-02-23 19:43   ` [PATCH v2 " Alexey Dobriyan
  2018-04-20  0:02 ` [PATCH " Andrew Morton
  3 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2018-02-21 20:06 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: linux-kernel

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!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/5] proc: make /proc/*/cmdline go through LSM
  2018-02-21 20:06 ` Andrew Morton
@ 2018-02-23 19:43   ` Alexey Dobriyan
  0 siblings, 0 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-02-23 19:43 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

/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,

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
  2018-02-21 19:23 [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Alexey Dobriyan
                   ` (2 preceding siblings ...)
  2018-02-21 20:06 ` Andrew Morton
@ 2018-04-20  0:02 ` Andrew Morton
  2018-04-20 19:25   ` Alexey Dobriyan
  3 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2018-04-20  0:02 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: linux-kernel

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline
  2018-02-21 19:30     ` [PATCH 4/5] proc: simpler iterations " Alexey Dobriyan
  2018-02-21 19:33       ` [PATCH 5/5] proc: deduplicate /proc/*/cmdline implementation Alexey Dobriyan
@ 2018-04-20  0:15       ` Andrew Morton
  2018-04-20 19:46         ` Alexey Dobriyan
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2018-04-20  0:15 UTC (permalink / raw)
  To: Alexey Dobriyan; +Cc: linux-kernel

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)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] proc: make /proc/*/cmdline go through LSM
  2018-04-20  0:02 ` [PATCH " Andrew Morton
@ 2018-04-20 19:25   ` Alexey Dobriyan
  0 siblings, 0 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-04-20 19:25 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline
  2018-04-20  0:15       ` [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline Andrew Morton
@ 2018-04-20 19:46         ` Alexey Dobriyan
  0 siblings, 0 replies; 13+ messages in thread
From: Alexey Dobriyan @ 2018-04-20 19:46 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-04-20 19:46 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-21 19:23 [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Alexey Dobriyan
2018-02-21 19:26 ` [PATCH 2/5] proc: more "unsigned int" in /proc/*/cmdline Alexey Dobriyan
2018-02-21 19:27   ` [PATCH 3/5] proc: somewhat simpler code for /proc/*/cmdline Alexey Dobriyan
2018-02-21 19:30     ` [PATCH 4/5] proc: simpler iterations " Alexey Dobriyan
2018-02-21 19:33       ` [PATCH 5/5] proc: deduplicate /proc/*/cmdline implementation Alexey Dobriyan
2018-04-20  0:15       ` [PATCH 4/5] proc: simpler iterations for /proc/*/cmdline Andrew Morton
2018-04-20 19:46         ` Alexey Dobriyan
2018-02-21 19:28 ` [PATCH 1/5] proc: make /proc/*/cmdline go through LSM Andy Shevchenko
2018-02-21 19:39   ` Alexey Dobriyan
2018-02-21 20:06 ` Andrew Morton
2018-02-23 19:43   ` [PATCH v2 " Alexey Dobriyan
2018-04-20  0:02 ` [PATCH " Andrew Morton
2018-04-20 19:25   ` Alexey Dobriyan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).