linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* regression: /proc/$pid/cmdline lacks trailing '\0' in 4.18-rc1
@ 2018-06-19  6:07 Michal Kubecek
  2018-06-19  9:22 ` Michal Kubecek
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2018-06-19  6:07 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexey Dobriyan, Linus Torvalds

In v4.18-rc1, /proc/$pid/cmdline is missing final null byte which used
to be there in v4.17 and older kernels:

4.17.1:
tweed:~ # cat /proc/self/cmdline | od -t c
0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
0000020   m   d   l   i   n   e  \0
0000027

4.18-rc1:
lion:~ # cat /proc/self/cmdline | od -t c
0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
0000020   m   d   l   i   n   e
0000026

The code has been rewritten quite a lot in 4.18-rc1 so I didn't find yet
where exactly does the change come from. Still looking.

Michal Kubecek

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

* Re: regression: /proc/$pid/cmdline lacks trailing '\0' in 4.18-rc1
  2018-06-19  6:07 regression: /proc/$pid/cmdline lacks trailing '\0' in 4.18-rc1 Michal Kubecek
@ 2018-06-19  9:22 ` Michal Kubecek
  2018-06-19 12:56   ` Michal Kubecek
  2018-06-19 16:17   ` Michal Kubecek
  0 siblings, 2 replies; 12+ messages in thread
From: Michal Kubecek @ 2018-06-19  9:22 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexey Dobriyan, Linus Torvalds

On Tue, Jun 19, 2018 at 08:07:55AM +0200, Michal Kubecek wrote:
> In v4.18-rc1, /proc/$pid/cmdline is missing final null byte which used
> to be there in v4.17 and older kernels:
> 
> 4.17.1:
> tweed:~ # cat /proc/self/cmdline | od -t c
> 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> 0000020   m   d   l   i   n   e  \0
> 0000027
> 
> 4.18-rc1:
> lion:~ # cat /proc/self/cmdline | od -t c
> 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> 0000020   m   d   l   i   n   e
> 0000026
> 
> The code has been rewritten quite a lot in 4.18-rc1 so I didn't find yet
> where exactly does the change come from. Still looking.

The issue was introduced by commit 5ab827189965 ("fs/proc: simplify and
clarify get_mm_cmdline() function"). The problem is that when looking
for the null character at or after args_end, strnlen() is used and it
returns the length _without_ the null character (if there is one) so
that we don't copy it.

I'll submit a patch once I test it.

Michal Kubecek

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

* Re: regression: /proc/$pid/cmdline lacks trailing '\0' in 4.18-rc1
  2018-06-19  9:22 ` Michal Kubecek
@ 2018-06-19 12:56   ` Michal Kubecek
  2018-06-19 16:17     ` [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline kbuild test robot
  2018-06-19 16:17   ` Michal Kubecek
  1 sibling, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2018-06-19 12:56 UTC (permalink / raw)
  To: linux-kernel; +Cc: Alexey Dobriyan, Linus Torvalds

On Tue, Jun 19, 2018 at 11:22:55AM +0200, Michal Kubecek wrote:
> On Tue, Jun 19, 2018 at 08:07:55AM +0200, Michal Kubecek wrote:
> > In v4.18-rc1, /proc/$pid/cmdline is missing final null byte which used
> > to be there in v4.17 and older kernels:
> > 
> > 4.17.1:
> > tweed:~ # cat /proc/self/cmdline | od -t c
> > 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> > 0000020   m   d   l   i   n   e  \0
> > 0000027
> > 
> > 4.18-rc1:
> > lion:~ # cat /proc/self/cmdline | od -t c
> > 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> > 0000020   m   d   l   i   n   e
> > 0000026
> > 
> > The code has been rewritten quite a lot in 4.18-rc1 so I didn't find yet
> > where exactly does the change come from. Still looking.
> 
> The issue was introduced by commit 5ab827189965 ("fs/proc: simplify and
> clarify get_mm_cmdline() function"). The problem is that when looking
> for the null character at or after args_end, strnlen() is used and it
> returns the length _without_ the null character (if there is one) so
> that we don't copy it.
> 
> I'll submit a patch once I test it.

It was more complicated than I thought. The patch below seems to resolve
the issue but I'll need to run more tests before I'm confident to submit
it officially.

Michal Kubecek



From: Michal Kubecek <mkubecek@suse.cz>
Date: Tue, 19 Jun 2018 14:47:23 +0200
Subject: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline

Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
trailing null character:

mike@lion:/tmp> cat /proc/self/cmdline | od -t c
0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
0000020   m   d   l   i   n   e
0000026

This is because strnlen() is used to search for the null character but it
returns the length without it so that we need to copy one byte more (but
only if we actually found a null character). And once we pass the null
character to userspace we need to make sure next read returns 0 (EOF).

This is another problem, even if rather theoretical one: if userspace seeks
to arg_end or past it, we only start checking for null character from that
position so that we may return random segment of environment rather then
EOF.

Resolve both by always starting no later than at arg_end-1 (so that we
always catch the right null character) but don't copy data until we reach
the requested start position.

Fixes: 5ab827189965 ("fs/proc: simplify and clarify get_mm_cmdline() function")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 fs/proc/base.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b6572944efc3..ac60405f5015 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -209,7 +209,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	unsigned long arg_start, arg_end, env_start, env_end;
-	unsigned long pos, len;
+	unsigned long req_pos, pos, len;
+	bool end_found = false;
 	char *page;
 
 	/* Check if process spawned far enough to have cmdline. */
@@ -236,25 +237,27 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 		env_start = env_end = arg_end;
 
 	/* We're not going to care if "*ppos" has high bits set */
-	pos = arg_start + *ppos;
+	req_pos = arg_start + *ppos;
 
 	/* .. but we do check the result is in the proper range */
-	if (pos < arg_start || pos >= env_end)
+	if (req_pos < arg_start || req_pos >= env_end)
 		return 0;
 
 	/* .. and we never go past env_end */
-	if (env_end - pos < count)
+	if (env_end - req_pos < count)
 		count = env_end - pos;
 
+	pos = min_t(unsigned long, req_pos, arg_end - 1);
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page)
 		return -ENOMEM;
 
 	len = 0;
-	while (count) {
+	while (count && !end_found) {
 		int got;
-		size_t size = min_t(size_t, PAGE_SIZE, count);
+		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
 
+		size = min_t(size_t, PAGE_SIZE, size);
 		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
 		if (got <= 0)
 			break;
@@ -276,11 +279,23 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 				n = arg_end - pos - 1;
 
 			/* Cut off at first NUL after 'n' */
-			got = n + strnlen(page+n, got-n);
+			n += strnlen(page+n, got-n);
+			got = min_t(int, got, n + 1);
+			end_found = !page[n];
 			if (!got)
 				break;
 		}
 
+		if (pos + got <= req_pos) {
+			/* got > 0 here so that pos always advances */
+			pos += got;
+			continue;
+		}
+
+		if (pos < req_pos) {
+			got -= (req_pos - pos);
+			pos = req_pos;
+		}
 		got -= copy_to_user(buf, page, got);
 		if (unlikely(!got)) {
 			if (!len)
-- 
2.17.1


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

* [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19  9:22 ` Michal Kubecek
  2018-06-19 12:56   ` Michal Kubecek
@ 2018-06-19 16:17   ` Michal Kubecek
  2018-06-19 16:28     ` [PATCH v2] " Michal Kubecek
                       ` (2 more replies)
  1 sibling, 3 replies; 12+ messages in thread
From: Michal Kubecek @ 2018-06-19 16:17 UTC (permalink / raw)
  To: linux-kernel; +Cc: Linus Torvalds, Alexey Dobriyan

Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
trailing null character:

mike@lion:/tmp> cat /proc/self/cmdline | od -t c
0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
0000020   m   d   l   i   n   e
0000026

This is because strnlen() is used to search for the null character but it
returns the length without it so that we need to copy one byte more (but
only if we actually found a null character). And once we pass the null
character to userspace we need to make sure next read returns 0 (EOF).

This is another problem, even if rather theoretical one: if userspace seeks
to arg_end or past it, we only start checking for null character from that
position so that we may return random segment of environment rather then
EOF.

Resolve both by always starting no later than at arg_end-1 (so that we
always catch the right null character) but don't copy data until we reach
the requested start position.

Fixes: 5ab827189965 ("fs/proc: simplify and clarify get_mm_cmdline() function")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 fs/proc/base.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b6572944efc3..8faafc70ee83 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -209,7 +209,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	unsigned long arg_start, arg_end, env_start, env_end;
-	unsigned long pos, len;
+	unsigned long req_pos, pos, len;
+	bool end_found = false;
 	char *page;
 
 	/* Check if process spawned far enough to have cmdline. */
@@ -236,25 +237,27 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 		env_start = env_end = arg_end;
 
 	/* We're not going to care if "*ppos" has high bits set */
-	pos = arg_start + *ppos;
+	req_pos = arg_start + *ppos;
 
 	/* .. but we do check the result is in the proper range */
-	if (pos < arg_start || pos >= env_end)
+	if (req_pos < arg_start || req_pos >= env_end)
 		return 0;
 
 	/* .. and we never go past env_end */
-	if (env_end - pos < count)
+	if (env_end - req_pos < count)
 		count = env_end - pos;
 
+	pos = min_t(unsigned long, req_pos, arg_end - 1);
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page)
 		return -ENOMEM;
 
 	len = 0;
-	while (count) {
+	while (count && !end_found) {
 		int got;
-		size_t size = min_t(size_t, PAGE_SIZE, count);
+		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
 
+		size = min_t(size_t, PAGE_SIZE, size);
 		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
 		if (got <= 0)
 			break;
@@ -276,12 +279,26 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 				n = arg_end - pos - 1;
 
 			/* Cut off at first NUL after 'n' */
-			got = n + strnlen(page+n, got-n);
+			n += strnlen(page + n, got - n);
+			got = min_t(int, got, n + 1);
+			end_found = !page[n];
 			if (!got)
 				break;
 		}
 
-		got -= copy_to_user(buf, page, got);
+		if (pos + got <= req_pos) {
+			/* got > 0 here so that pos always advances */
+			pos += got;
+			continue;
+		}
+
+		if (pos < req_pos) {
+			got -= (req_pos - pos);
+			got -= copy_to_user(buf, page + req_pos - pos, got);
+			pos = req_pos;
+		} else {
+			got -= copy_to_user(buf, page, got);
+		}
 		if (unlikely(!got)) {
 			if (!len)
 				len = -EFAULT;
-- 
2.17.1


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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 12:56   ` Michal Kubecek
@ 2018-06-19 16:17     ` kbuild test robot
  0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-06-19 16:17 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: kbuild-all, linux-kernel, Alexey Dobriyan, Linus Torvalds

[-- Attachment #1: Type: text/plain, Size: 8614 bytes --]

Hi Michal,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.18-rc1 next-20180619]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Kubecek/proc-add-missing-0-back-to-proc-pid-cmdline/20180619-210800
config: x86_64-randconfig-x015-201824 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   fs/proc/base.c: In function 'proc_pid_cmdline_read':
>> fs/proc/base.c:248:9: warning: 'pos' may be used uninitialized in this function [-Wmaybe-uninitialized]
      count = env_end - pos;
      ~~~~~~^~~~~~~~~~~~~~~
   fs/proc/base.c:212:25: note: 'pos' was declared here
     unsigned long req_pos, pos, len;
                            ^~~

vim +/pos +248 fs/proc/base.c

^1da177e Linus Torvalds  2005-04-16  207  
e4b4e441 Linus Torvalds  2018-05-17  208  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
5ab82718 Linus Torvalds  2018-05-17  209  			      size_t count, loff_t *ppos)
^1da177e Linus Torvalds  2005-04-16  210  {
c2c0bb44 Alexey Dobriyan 2015-06-25  211  	unsigned long arg_start, arg_end, env_start, env_end;
bf62f801 Michal Kubecek  2018-06-19  212  	unsigned long req_pos, pos, len;
bf62f801 Michal Kubecek  2018-06-19  213  	bool end_found = false;
5ab82718 Linus Torvalds  2018-05-17  214  	char *page;
c2c0bb44 Alexey Dobriyan 2015-06-25  215  
c2c0bb44 Alexey Dobriyan 2015-06-25  216  	/* Check if process spawned far enough to have cmdline. */
e4b4e441 Linus Torvalds  2018-05-17  217  	if (!mm->env_end)
e4b4e441 Linus Torvalds  2018-05-17  218  		return 0;
c2c0bb44 Alexey Dobriyan 2015-06-25  219  
88aa7cc6 Yang Shi        2018-06-07  220  	spin_lock(&mm->arg_lock);
c2c0bb44 Alexey Dobriyan 2015-06-25  221  	arg_start = mm->arg_start;
c2c0bb44 Alexey Dobriyan 2015-06-25  222  	arg_end = mm->arg_end;
c2c0bb44 Alexey Dobriyan 2015-06-25  223  	env_start = mm->env_start;
c2c0bb44 Alexey Dobriyan 2015-06-25  224  	env_end = mm->env_end;
88aa7cc6 Yang Shi        2018-06-07  225  	spin_unlock(&mm->arg_lock);
c2c0bb44 Alexey Dobriyan 2015-06-25  226  
5ab82718 Linus Torvalds  2018-05-17  227  	if (arg_start >= arg_end)
5ab82718 Linus Torvalds  2018-05-17  228  		return 0;
6a6cbe75 Alexey Dobriyan 2018-06-07  229  
2ca66ff7 Alexey Dobriyan 2014-08-08  230  	/*
5ab82718 Linus Torvalds  2018-05-17  231  	 * We have traditionally allowed the user to re-write
5ab82718 Linus Torvalds  2018-05-17  232  	 * the argument strings and overflow the end result
5ab82718 Linus Torvalds  2018-05-17  233  	 * into the environment section. But only do that if
5ab82718 Linus Torvalds  2018-05-17  234  	 * the environment area is contiguous to the arguments.
2ca66ff7 Alexey Dobriyan 2014-08-08  235  	 */
5ab82718 Linus Torvalds  2018-05-17  236  	if (env_start != arg_end || env_start >= env_end)
5ab82718 Linus Torvalds  2018-05-17  237  		env_start = env_end = arg_end;
3cb4e162 Alexey Dobriyan 2018-06-07  238  
5ab82718 Linus Torvalds  2018-05-17  239  	/* We're not going to care if "*ppos" has high bits set */
bf62f801 Michal Kubecek  2018-06-19  240  	req_pos = arg_start + *ppos;
c2c0bb44 Alexey Dobriyan 2015-06-25  241  
5ab82718 Linus Torvalds  2018-05-17  242  	/* .. but we do check the result is in the proper range */
bf62f801 Michal Kubecek  2018-06-19  243  	if (req_pos < arg_start || req_pos >= env_end)
5ab82718 Linus Torvalds  2018-05-17  244  		return 0;
3cb4e162 Alexey Dobriyan 2018-06-07  245  
5ab82718 Linus Torvalds  2018-05-17  246  	/* .. and we never go past env_end */
bf62f801 Michal Kubecek  2018-06-19  247  	if (env_end - req_pos < count)
5ab82718 Linus Torvalds  2018-05-17 @248  		count = env_end - pos;
c2c0bb44 Alexey Dobriyan 2015-06-25  249  
bf62f801 Michal Kubecek  2018-06-19  250  	pos = min_t(unsigned long, req_pos, arg_end - 1);
5ab82718 Linus Torvalds  2018-05-17  251  	page = (char *)__get_free_page(GFP_KERNEL);
5ab82718 Linus Torvalds  2018-05-17  252  	if (!page)
5ab82718 Linus Torvalds  2018-05-17  253  		return -ENOMEM;
5ab82718 Linus Torvalds  2018-05-17  254  
5ab82718 Linus Torvalds  2018-05-17  255  	len = 0;
bf62f801 Michal Kubecek  2018-06-19  256  	while (count && !end_found) {
5ab82718 Linus Torvalds  2018-05-17  257  		int got;
bf62f801 Michal Kubecek  2018-06-19  258  		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
5ab82718 Linus Torvalds  2018-05-17  259  
bf62f801 Michal Kubecek  2018-06-19  260  		size = min_t(size_t, PAGE_SIZE, size);
5ab82718 Linus Torvalds  2018-05-17  261  		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
5ab82718 Linus Torvalds  2018-05-17  262  		if (got <= 0)
5ab82718 Linus Torvalds  2018-05-17  263  			break;
5ab82718 Linus Torvalds  2018-05-17  264  
5ab82718 Linus Torvalds  2018-05-17  265  		/* Don't walk past a NUL character once you hit arg_end */
5ab82718 Linus Torvalds  2018-05-17  266  		if (pos + got >= arg_end) {
5ab82718 Linus Torvalds  2018-05-17  267  			int n = 0;
c2c0bb44 Alexey Dobriyan 2015-06-25  268  
c2c0bb44 Alexey Dobriyan 2015-06-25  269  			/*
5ab82718 Linus Torvalds  2018-05-17  270  			 * If we started before 'arg_end' but ended up
5ab82718 Linus Torvalds  2018-05-17  271  			 * at or after it, we start the NUL character
5ab82718 Linus Torvalds  2018-05-17  272  			 * check at arg_end-1 (where we expect the normal
5ab82718 Linus Torvalds  2018-05-17  273  			 * EOF to be).
5ab82718 Linus Torvalds  2018-05-17  274  			 *
5ab82718 Linus Torvalds  2018-05-17  275  			 * NOTE! This is smaller than 'got', because
5ab82718 Linus Torvalds  2018-05-17  276  			 * pos + got >= arg_end
c2c0bb44 Alexey Dobriyan 2015-06-25  277  			 */
5ab82718 Linus Torvalds  2018-05-17  278  			if (pos < arg_end)
5ab82718 Linus Torvalds  2018-05-17  279  				n = arg_end - pos - 1;
c2c0bb44 Alexey Dobriyan 2015-06-25  280  
5ab82718 Linus Torvalds  2018-05-17  281  			/* Cut off at first NUL after 'n' */
bf62f801 Michal Kubecek  2018-06-19  282  			n += strnlen(page+n, got-n);
bf62f801 Michal Kubecek  2018-06-19  283  			got = min_t(int, got, n + 1);
bf62f801 Michal Kubecek  2018-06-19  284  			end_found = !page[n];
5ab82718 Linus Torvalds  2018-05-17  285  			if (!got)
5ab82718 Linus Torvalds  2018-05-17  286  				break;
c2c0bb44 Alexey Dobriyan 2015-06-25  287  		}
c2c0bb44 Alexey Dobriyan 2015-06-25  288  
bf62f801 Michal Kubecek  2018-06-19  289  		if (pos + got <= req_pos) {
bf62f801 Michal Kubecek  2018-06-19  290  			/* got > 0 here so that pos always advances */
bf62f801 Michal Kubecek  2018-06-19  291  			pos += got;
bf62f801 Michal Kubecek  2018-06-19  292  			continue;
bf62f801 Michal Kubecek  2018-06-19  293  		}
bf62f801 Michal Kubecek  2018-06-19  294  
bf62f801 Michal Kubecek  2018-06-19  295  		if (pos < req_pos) {
bf62f801 Michal Kubecek  2018-06-19  296  			got -= (req_pos - pos);
bf62f801 Michal Kubecek  2018-06-19  297  			pos = req_pos;
bf62f801 Michal Kubecek  2018-06-19  298  		}
5ab82718 Linus Torvalds  2018-05-17  299  		got -= copy_to_user(buf, page, got);
5ab82718 Linus Torvalds  2018-05-17  300  		if (unlikely(!got)) {
5ab82718 Linus Torvalds  2018-05-17  301  			if (!len)
5ab82718 Linus Torvalds  2018-05-17  302  				len = -EFAULT;
5ab82718 Linus Torvalds  2018-05-17  303  			break;
c2c0bb44 Alexey Dobriyan 2015-06-25  304  		}
5ab82718 Linus Torvalds  2018-05-17  305  		pos += got;
5ab82718 Linus Torvalds  2018-05-17  306  		buf += got;
5ab82718 Linus Torvalds  2018-05-17  307  		len += got;
5ab82718 Linus Torvalds  2018-05-17  308  		count -= got;
c2c0bb44 Alexey Dobriyan 2015-06-25  309  	}
c2c0bb44 Alexey Dobriyan 2015-06-25  310  
c2c0bb44 Alexey Dobriyan 2015-06-25  311  	free_page((unsigned long)page);
5ab82718 Linus Torvalds  2018-05-17  312  	return len;
c2c0bb44 Alexey Dobriyan 2015-06-25  313  }
c2c0bb44 Alexey Dobriyan 2015-06-25  314  

:::::: The code at line 248 was first introduced by commit
:::::: 5ab8271899658042fabc5ae7e6a99066a210bc0e fs/proc: simplify and clarify get_mm_cmdline() function

:::::: TO: Linus Torvalds <torvalds@linux-foundation.org>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29179 bytes --]

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

* [PATCH v2] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 16:17   ` Michal Kubecek
@ 2018-06-19 16:28     ` Michal Kubecek
  2018-06-19 17:14       ` Alexey Dobriyan
  2018-06-19 21:56     ` [PATCH] " Linus Torvalds
  2018-06-20  9:22     ` Dan Carpenter
  2 siblings, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2018-06-19 16:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: Linus Torvalds, Alexey Dobriyan

Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
trailing null character:

mike@lion:/tmp> cat /proc/self/cmdline | od -t c
0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
0000020   m   d   l   i   n   e
0000026

This is because strnlen() is used to search for the null character but it
returns the length without it so that we need to copy one byte more (but
only if we actually found a null character). And once we pass the null
character to userspace we need to make sure next read returns 0 (EOF).

This is another problem, even if rather theoretical one: if userspace seeks
to arg_end or past it, we only start checking for null character from that
position so that we may return random segment of environment rather then
EOF.

Resolve both by always starting no later than at arg_end-1 (so that we
always catch the right null character) but don't copy data until we reach
the requested start position.

Fixes: 5ab827189965 ("fs/proc: simplify and clarify get_mm_cmdline() function")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 fs/proc/base.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b6572944efc3..dc4d7d6818f9 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -209,7 +209,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	unsigned long arg_start, arg_end, env_start, env_end;
-	unsigned long pos, len;
+	unsigned long req_pos, pos, len;
+	bool end_found = false;
 	char *page;
 
 	/* Check if process spawned far enough to have cmdline. */
@@ -236,25 +237,27 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 		env_start = env_end = arg_end;
 
 	/* We're not going to care if "*ppos" has high bits set */
-	pos = arg_start + *ppos;
+	req_pos = arg_start + *ppos;
 
 	/* .. but we do check the result is in the proper range */
-	if (pos < arg_start || pos >= env_end)
+	if (req_pos < arg_start || req_pos >= env_end)
 		return 0;
 
 	/* .. and we never go past env_end */
-	if (env_end - pos < count)
-		count = env_end - pos;
+	if (env_end - req_pos < count)
+		count = env_end - req_pos;
 
+	pos = min_t(unsigned long, req_pos, arg_end - 1);
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page)
 		return -ENOMEM;
 
 	len = 0;
-	while (count) {
+	while (count && !end_found) {
 		int got;
-		size_t size = min_t(size_t, PAGE_SIZE, count);
+		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
 
+		size = min_t(size_t, PAGE_SIZE, size);
 		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
 		if (got <= 0)
 			break;
@@ -276,12 +279,26 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 				n = arg_end - pos - 1;
 
 			/* Cut off at first NUL after 'n' */
-			got = n + strnlen(page+n, got-n);
+			n += strnlen(page + n, got - n);
+			got = min_t(int, got, n + 1);
+			end_found = !page[n];
 			if (!got)
 				break;
 		}
 
-		got -= copy_to_user(buf, page, got);
+		if (pos + got <= req_pos) {
+			/* got > 0 here so that pos always advances */
+			pos += got;
+			continue;
+		}
+
+		if (pos < req_pos) {
+			got -= (req_pos - pos);
+			got -= copy_to_user(buf, page + req_pos - pos, got);
+			pos = req_pos;
+		} else {
+			got -= copy_to_user(buf, page, got);
+		}
 		if (unlikely(!got)) {
 			if (!len)
 				len = -EFAULT;
-- 
2.17.1


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

* Re: [PATCH v2] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 16:28     ` [PATCH v2] " Michal Kubecek
@ 2018-06-19 17:14       ` Alexey Dobriyan
  0 siblings, 0 replies; 12+ messages in thread
From: Alexey Dobriyan @ 2018-06-19 17:14 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: linux-kernel, Linus Torvalds

On Tue, Jun 19, 2018 at 06:28:40PM +0200, Michal Kubecek wrote:
> Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
> trailing null character:
> 
> mike@lion:/tmp> cat /proc/self/cmdline | od -t c
> 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> 0000020   m   d   l   i   n   e
> 0000026
> 
> This is because strnlen() is used to search for the null character but it
> returns the length without it so that we need to copy one byte more (but
> only if we actually found a null character). And once we pass the null
> character to userspace we need to make sure next read returns 0 (EOF).
> 
> This is another problem, even if rather theoretical one: if userspace seeks
> to arg_end or past it, we only start checking for null character from that
> position so that we may return random segment of environment rather then
> EOF.
> 
> Resolve both by always starting no later than at arg_end-1 (so that we
> always catch the right null character) but don't copy data until we reach
> the requested start position.

See what's happening?

Code was made allegedly simpler by removing special case for "c == '\0'"
but that didn't work and now you're adding complexity back with another
variable and branches.

> Fixes: 5ab827189965 ("fs/proc: simplify and clarify get_mm_cmdline() function")

> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -209,7 +209,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
>  	unsigned long arg_start, arg_end, env_start, env_end;
> -	unsigned long pos, len;
> +	unsigned long req_pos, pos, len;
> +	bool end_found = false;
>  	char *page;
>  
>  	/* Check if process spawned far enough to have cmdline. */
> @@ -236,25 +237,27 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  		env_start = env_end = arg_end;
>  
>  	/* We're not going to care if "*ppos" has high bits set */
> -	pos = arg_start + *ppos;
> +	req_pos = arg_start + *ppos;
>  
>  	/* .. but we do check the result is in the proper range */
> -	if (pos < arg_start || pos >= env_end)
> +	if (req_pos < arg_start || req_pos >= env_end)
>  		return 0;
>  
>  	/* .. and we never go past env_end */
> -	if (env_end - pos < count)
> -		count = env_end - pos;
> +	if (env_end - req_pos < count)
> +		count = env_end - req_pos;
>  
> +	pos = min_t(unsigned long, req_pos, arg_end - 1);
>  	page = (char *)__get_free_page(GFP_KERNEL);
>  	if (!page)
>  		return -ENOMEM;
>  
>  	len = 0;
> -	while (count) {
> +	while (count && !end_found) {
>  		int got;
> -		size_t size = min_t(size_t, PAGE_SIZE, count);
> +		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
>  
> +		size = min_t(size_t, PAGE_SIZE, size);
>  		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
>  		if (got <= 0)
>  			break;
> @@ -276,12 +279,26 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  				n = arg_end - pos - 1;
>  
>  			/* Cut off at first NUL after 'n' */
> -			got = n + strnlen(page+n, got-n);
> +			n += strnlen(page + n, got - n);
> +			got = min_t(int, got, n + 1);
> +			end_found = !page[n];
>  			if (!got)
>  				break;
>  		}
>  
> -		got -= copy_to_user(buf, page, got);
> +		if (pos + got <= req_pos) {
> +			/* got > 0 here so that pos always advances */
> +			pos += got;
> +			continue;
> +		}
> +
> +		if (pos < req_pos) {
> +			got -= (req_pos - pos);
> +			got -= copy_to_user(buf, page + req_pos - pos, got);
> +			pos = req_pos;
> +		} else {
> +			got -= copy_to_user(buf, page, got);
> +		}
>  		if (unlikely(!got)) {
>  			if (!len)
>  				len = -EFAULT;

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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 16:17   ` Michal Kubecek
  2018-06-19 16:28     ` [PATCH v2] " Michal Kubecek
@ 2018-06-19 21:56     ` Linus Torvalds
  2018-06-20  5:08       ` Michal Kubecek
  2018-06-20  9:22     ` Dan Carpenter
  2 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2018-06-19 21:56 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: Linux Kernel Mailing List, Alexey Dobriyan

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

On Wed, Jun 20, 2018 at 1:24 AM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
> trailing null character:
>
> mike@lion:/tmp> cat /proc/self/cmdline | od -t c
> 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> 0000020   m   d   l   i   n   e
> 0000026

Thanks, and obviously right you are.

That said, I'm not a fan of your patch. I'd much rather just tweak the
"strnlen()" logic a bit instead, and make the rule be that when we go
into the "slop" area, we always include the last byte of the "real"
argv area.

That limits the slop to a page (well, one byte less, since we want the
one byte of non-slop), but honestly, a page for *everything* was what
we used to do originally, so..

How does the attached patch work for you?

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1856 bytes --]

 fs/proc/base.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b6572944efc3..aaffc0c30216 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -235,6 +235,10 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 	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;
 
@@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 	while (count) {
 		int got;
 		size_t size = min_t(size_t, PAGE_SIZE, count);
+		long offset;
 
-		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
-		if (got <= 0)
+		/*
+		 * Are we already starting past the official end?
+		 * We always include the last byte that is *supposed*
+		 * to be NUL
+		 */
+		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) {
@@ -276,12 +289,17 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 				n = arg_end - pos - 1;
 
 			/* Cut off at first NUL after 'n' */
-			got = n + strnlen(page+n, got-n);
-			if (!got)
+			got = n + strnlen(page+n, offset+got-n);
+			if (got < offset)
 				break;
+			got -= offset;
+
+			/* Include the NUL if it existed */
+			if (got < size)
+				got++;
 		}
 
-		got -= copy_to_user(buf, page, got);
+		got -= copy_to_user(buf, page+offset, got);
 		if (unlikely(!got)) {
 			if (!len)
 				len = -EFAULT;

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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 21:56     ` [PATCH] " Linus Torvalds
@ 2018-06-20  5:08       ` Michal Kubecek
  2018-06-20  5:51         ` Linus Torvalds
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Kubecek @ 2018-06-20  5:08 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Alexey Dobriyan

On Wed, Jun 20, 2018 at 06:56:04AM +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 1:24 AM Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> > Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
> > trailing null character:
> >
> > mike@lion:/tmp> cat /proc/self/cmdline | od -t c
> > 0000000   c   a   t  \0   /   p   r   o   c   /   s   e   l   f   /   c
> > 0000020   m   d   l   i   n   e
> > 0000026
> 
> Thanks, and obviously right you are.
> 
> That said, I'm not a fan of your patch. I'd much rather just tweak the
> "strnlen()" logic a bit instead, and make the rule be that when we go
> into the "slop" area, we always include the last byte of the "real"
> argv area.
> 
> That limits the slop to a page (well, one byte less, since we want the
> one byte of non-slop), but honestly, a page for *everything* was what
> we used to do originally, so..

Yes, that should be enough for real life applications.

> How does the attached patch work for you?

I haven't tested it yet but it looks good except this:

> @@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  	while (count) {
>  		int got;
>  		size_t size = min_t(size_t, PAGE_SIZE, count);

We limit size to be at most PAGE_SIZE here.

> +		long offset;
>  
> -		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
> -		if (got <= 0)
> +		/*
> +		 * Are we already starting past the official end?
> +		 * We always include the last byte that is *supposed*
> +		 * to be NUL
> +		 */
> +		offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> +
> +		got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);

But here we read (size + offset) bytes which may be more than PAGE_SIZE.
I guess it should rather be

	size_t size;
...
	offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
	size = min_t(size_t, PAGE_SIZE - offset, count);

We already made sure that offset < PAGE_SIZE so that size will be at
least 1.

Michal Kubecek

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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-20  5:08       ` Michal Kubecek
@ 2018-06-20  5:51         ` Linus Torvalds
  2018-06-20  6:09           ` Michal Kubecek
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2018-06-20  5:51 UTC (permalink / raw)
  To: Michal Kubecek; +Cc: Linux Kernel Mailing List, Alexey Dobriyan

On Wed, Jun 20, 2018 at 2:08 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
>
> > @@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> >       while (count) {
> >               int got;
> >               size_t size = min_t(size_t, PAGE_SIZE, count);
>
> We limit size to be at most PAGE_SIZE here.
>
> > +             long offset;
> >
> > -             got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
> > -             if (got <= 0)
> > +             /*
> > +              * Are we already starting past the official end?
> > +              * We always include the last byte that is *supposed*
> > +              * to be NUL
> > +              */
> > +             offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> > +
> > +             got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);
>
> But here we read (size + offset) bytes which may be more than PAGE_SIZE.

Actually, no. We limit size not just to PAGE_SIZE, but to count as well.

And there's *another* limit on 'count' that you missed, namely this part:

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

coupled with

        /* .. and we never go past env_end */
        if (env_end - pos < count)
                count = env_end - pos;

so we know that "pos + size" can never be larger than "arg_end + PAGE_SIZE - 1"

And then:

                offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;

means that "offset" will be bigger than zero only if "pos > arg_end-1".

So let's ignore all other cases, and just say that we care about that
case where 'offset' can be non-zero.

So we have

   offset = pos - arg_end +1

(from the above initialization of offset), but we also know that

   pos + count <= end_end

and since we've limited end_end to "arg_end + PAGE_SIZE -1" we have

   pos + count <= arg_end + PAGE_SIZE -1

agreed?

Now, we can do some math on the above. Re-write that "offset = .." equation as

   pos = arg_end + offset - 1

And the same time we can  take that "pos + count" inequality, and
replacing "pos", we get

   arg_end + offset - 1 + count <= arg_end + PAGE_SIZE -1

and then we can remove "arg_end - 1" from both sides, and get

   offset+count <= PAGE_SIZE

agreed?

              Linus

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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-20  5:51         ` Linus Torvalds
@ 2018-06-20  6:09           ` Michal Kubecek
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Kubecek @ 2018-06-20  6:09 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel Mailing List, Alexey Dobriyan

On Wed, Jun 20, 2018 at 02:51:23PM +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 2:08 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> >
> >
> > > @@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> > >       while (count) {
> > >               int got;
> > >               size_t size = min_t(size_t, PAGE_SIZE, count);
> >
> > We limit size to be at most PAGE_SIZE here.
> >
> > > +             long offset;
> > >
> > > -             got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
> > > -             if (got <= 0)
> > > +             /*
> > > +              * Are we already starting past the official end?
> > > +              * We always include the last byte that is *supposed*
> > > +              * to be NUL
> > > +              */
> > > +             offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> > > +
> > > +             got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);
> >
> > But here we read (size + offset) bytes which may be more than PAGE_SIZE.
> 
> Actually, no. We limit size not just to PAGE_SIZE, but to count as well.
> 
> And there's *another* limit on 'count' that you missed, namely this part:
> 
>         /* .. 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;
> 
> coupled with
> 
>         /* .. and we never go past env_end */
>         if (env_end - pos < count)
>                 count = env_end - pos;
> 
> so we know that "pos + size" can never be larger than "arg_end + PAGE_SIZE - 1"
> 
> And then:
> 
>                 offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> 
> means that "offset" will be bigger than zero only if "pos > arg_end-1".
> 
> So let's ignore all other cases, and just say that we care about that
> case where 'offset' can be non-zero.
> 
> So we have
> 
>    offset = pos - arg_end +1
> 
> (from the above initialization of offset), but we also know that
> 
>    pos + count <= end_end
> 
> and since we've limited end_end to "arg_end + PAGE_SIZE -1" we have
> 
>    pos + count <= arg_end + PAGE_SIZE -1
> 
> agreed?
> 
> Now, we can do some math on the above. Re-write that "offset = .." equation as
> 
>    pos = arg_end + offset - 1
> 
> And the same time we can  take that "pos + count" inequality, and
> replacing "pos", we get
> 
>    arg_end + offset - 1 + count <= arg_end + PAGE_SIZE -1
> 
> and then we can remove "arg_end - 1" from both sides, and get
> 
>    offset+count <= PAGE_SIZE
> 
> agreed?

Right... I missed the trick where env_end is no longer real env_end when
it's used to limit count.

And the tests look good too so

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
Tested-by: Michal Kubecek <mkubecek@suse.cz>

Michal Kubecek


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

* Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline
  2018-06-19 16:17   ` Michal Kubecek
  2018-06-19 16:28     ` [PATCH v2] " Michal Kubecek
  2018-06-19 21:56     ` [PATCH] " Linus Torvalds
@ 2018-06-20  9:22     ` Dan Carpenter
  2 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2018-06-20  9:22 UTC (permalink / raw)
  To: kbuild, Michal Kubecek
  Cc: kbuild-all, linux-kernel, Linus Torvalds, Alexey Dobriyan

Hi Michal,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Michal-Kubecek/proc-add-missing-0-back-to-proc-pid-cmdline/20180620-015310

New smatch warnings:
fs/proc/base.c:248 get_mm_cmdline() error: uninitialized symbol 'pos'.

Old smatch warnings:
fs/proc/base.c:1882 proc_fill_cache() error: 'child' dereferencing possible ERR_PTR()

# https://github.com/0day-ci/linux/commit/b305ec6298033adaaac8c8598028f0ca1a1234b9
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout b305ec6298033adaaac8c8598028f0ca1a1234b9
vim +/pos +248 fs/proc/base.c

^1da177e Linus Torvalds  2005-04-16  207  
e4b4e441 Linus Torvalds  2018-05-17  208  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
5ab82718 Linus Torvalds  2018-05-17  209  			      size_t count, loff_t *ppos)
^1da177e Linus Torvalds  2005-04-16  210  {
c2c0bb44 Alexey Dobriyan 2015-06-25  211  	unsigned long arg_start, arg_end, env_start, env_end;
b305ec62 Michal Kubecek  2018-06-19  212  	unsigned long req_pos, pos, len;
b305ec62 Michal Kubecek  2018-06-19  213  	bool end_found = false;
5ab82718 Linus Torvalds  2018-05-17  214  	char *page;
c2c0bb44 Alexey Dobriyan 2015-06-25  215  
c2c0bb44 Alexey Dobriyan 2015-06-25  216  	/* Check if process spawned far enough to have cmdline. */
e4b4e441 Linus Torvalds  2018-05-17  217  	if (!mm->env_end)
e4b4e441 Linus Torvalds  2018-05-17  218  		return 0;
c2c0bb44 Alexey Dobriyan 2015-06-25  219  
88aa7cc6 Yang Shi        2018-06-07  220  	spin_lock(&mm->arg_lock);
c2c0bb44 Alexey Dobriyan 2015-06-25  221  	arg_start = mm->arg_start;
c2c0bb44 Alexey Dobriyan 2015-06-25  222  	arg_end = mm->arg_end;
c2c0bb44 Alexey Dobriyan 2015-06-25  223  	env_start = mm->env_start;
c2c0bb44 Alexey Dobriyan 2015-06-25  224  	env_end = mm->env_end;
88aa7cc6 Yang Shi        2018-06-07  225  	spin_unlock(&mm->arg_lock);
c2c0bb44 Alexey Dobriyan 2015-06-25  226  
5ab82718 Linus Torvalds  2018-05-17  227  	if (arg_start >= arg_end)
5ab82718 Linus Torvalds  2018-05-17  228  		return 0;
6a6cbe75 Alexey Dobriyan 2018-06-07  229  
2ca66ff7 Alexey Dobriyan 2014-08-08  230  	/*
5ab82718 Linus Torvalds  2018-05-17  231  	 * We have traditionally allowed the user to re-write
5ab82718 Linus Torvalds  2018-05-17  232  	 * the argument strings and overflow the end result
5ab82718 Linus Torvalds  2018-05-17  233  	 * into the environment section. But only do that if
5ab82718 Linus Torvalds  2018-05-17  234  	 * the environment area is contiguous to the arguments.
2ca66ff7 Alexey Dobriyan 2014-08-08  235  	 */
5ab82718 Linus Torvalds  2018-05-17  236  	if (env_start != arg_end || env_start >= env_end)
5ab82718 Linus Torvalds  2018-05-17  237  		env_start = env_end = arg_end;
3cb4e162 Alexey Dobriyan 2018-06-07  238  
5ab82718 Linus Torvalds  2018-05-17  239  	/* We're not going to care if "*ppos" has high bits set */
b305ec62 Michal Kubecek  2018-06-19  240  	req_pos = arg_start + *ppos;
c2c0bb44 Alexey Dobriyan 2015-06-25  241  
5ab82718 Linus Torvalds  2018-05-17  242  	/* .. but we do check the result is in the proper range */
b305ec62 Michal Kubecek  2018-06-19  243  	if (req_pos < arg_start || req_pos >= env_end)
5ab82718 Linus Torvalds  2018-05-17  244  		return 0;
3cb4e162 Alexey Dobriyan 2018-06-07  245  
5ab82718 Linus Torvalds  2018-05-17  246  	/* .. and we never go past env_end */
b305ec62 Michal Kubecek  2018-06-19  247  	if (env_end - req_pos < count)
5ab82718 Linus Torvalds  2018-05-17 @248  		count = env_end - pos;
c2c0bb44 Alexey Dobriyan 2015-06-25  249  
b305ec62 Michal Kubecek  2018-06-19  250  	pos = min_t(unsigned long, req_pos, arg_end - 1);
5ab82718 Linus Torvalds  2018-05-17  251  	page = (char *)__get_free_page(GFP_KERNEL);
5ab82718 Linus Torvalds  2018-05-17  252  	if (!page)
5ab82718 Linus Torvalds  2018-05-17  253  		return -ENOMEM;
5ab82718 Linus Torvalds  2018-05-17  254  
5ab82718 Linus Torvalds  2018-05-17  255  	len = 0;
b305ec62 Michal Kubecek  2018-06-19  256  	while (count && !end_found) {
5ab82718 Linus Torvalds  2018-05-17  257  		int got;
b305ec62 Michal Kubecek  2018-06-19  258  		size_t size = count + (pos < req_pos ? req_pos - pos : 0);
5ab82718 Linus Torvalds  2018-05-17  259  
b305ec62 Michal Kubecek  2018-06-19  260  		size = min_t(size_t, PAGE_SIZE, size);
5ab82718 Linus Torvalds  2018-05-17  261  		got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
5ab82718 Linus Torvalds  2018-05-17  262  		if (got <= 0)
5ab82718 Linus Torvalds  2018-05-17  263  			break;
5ab82718 Linus Torvalds  2018-05-17  264  
5ab82718 Linus Torvalds  2018-05-17  265  		/* Don't walk past a NUL character once you hit arg_end */
5ab82718 Linus Torvalds  2018-05-17  266  		if (pos + got >= arg_end) {
5ab82718 Linus Torvalds  2018-05-17  267  			int n = 0;
c2c0bb44 Alexey Dobriyan 2015-06-25  268  
c2c0bb44 Alexey Dobriyan 2015-06-25  269  			/*
5ab82718 Linus Torvalds  2018-05-17  270  			 * If we started before 'arg_end' but ended up
5ab82718 Linus Torvalds  2018-05-17  271  			 * at or after it, we start the NUL character
5ab82718 Linus Torvalds  2018-05-17  272  			 * check at arg_end-1 (where we expect the normal
5ab82718 Linus Torvalds  2018-05-17  273  			 * EOF to be).
5ab82718 Linus Torvalds  2018-05-17  274  			 *
5ab82718 Linus Torvalds  2018-05-17  275  			 * NOTE! This is smaller than 'got', because
5ab82718 Linus Torvalds  2018-05-17  276  			 * pos + got >= arg_end
c2c0bb44 Alexey Dobriyan 2015-06-25  277  			 */
5ab82718 Linus Torvalds  2018-05-17  278  			if (pos < arg_end)
5ab82718 Linus Torvalds  2018-05-17  279  				n = arg_end - pos - 1;
c2c0bb44 Alexey Dobriyan 2015-06-25  280  
5ab82718 Linus Torvalds  2018-05-17  281  			/* Cut off at first NUL after 'n' */
b305ec62 Michal Kubecek  2018-06-19  282  			n += strnlen(page + n, got - n);
b305ec62 Michal Kubecek  2018-06-19  283  			got = min_t(int, got, n + 1);
b305ec62 Michal Kubecek  2018-06-19  284  			end_found = !page[n];
5ab82718 Linus Torvalds  2018-05-17  285  			if (!got)
5ab82718 Linus Torvalds  2018-05-17  286  				break;
c2c0bb44 Alexey Dobriyan 2015-06-25  287  		}
c2c0bb44 Alexey Dobriyan 2015-06-25  288  
b305ec62 Michal Kubecek  2018-06-19  289  		if (pos + got <= req_pos) {
b305ec62 Michal Kubecek  2018-06-19  290  			/* got > 0 here so that pos always advances */
b305ec62 Michal Kubecek  2018-06-19  291  			pos += got;
b305ec62 Michal Kubecek  2018-06-19  292  			continue;
b305ec62 Michal Kubecek  2018-06-19  293  		}
b305ec62 Michal Kubecek  2018-06-19  294  
b305ec62 Michal Kubecek  2018-06-19  295  		if (pos < req_pos) {
b305ec62 Michal Kubecek  2018-06-19  296  			got -= (req_pos - pos);
b305ec62 Michal Kubecek  2018-06-19  297  			got -= copy_to_user(buf, page + req_pos - pos, got);
b305ec62 Michal Kubecek  2018-06-19  298  			pos = req_pos;
b305ec62 Michal Kubecek  2018-06-19  299  		} else {
5ab82718 Linus Torvalds  2018-05-17  300  			got -= copy_to_user(buf, page, got);
b305ec62 Michal Kubecek  2018-06-19  301  		}
5ab82718 Linus Torvalds  2018-05-17  302  		if (unlikely(!got)) {
5ab82718 Linus Torvalds  2018-05-17  303  			if (!len)
5ab82718 Linus Torvalds  2018-05-17  304  				len = -EFAULT;
5ab82718 Linus Torvalds  2018-05-17  305  			break;
c2c0bb44 Alexey Dobriyan 2015-06-25  306  		}
5ab82718 Linus Torvalds  2018-05-17  307  		pos += got;
5ab82718 Linus Torvalds  2018-05-17  308  		buf += got;
5ab82718 Linus Torvalds  2018-05-17  309  		len += got;
5ab82718 Linus Torvalds  2018-05-17  310  		count -= got;
c2c0bb44 Alexey Dobriyan 2015-06-25  311  	}
c2c0bb44 Alexey Dobriyan 2015-06-25  312  
c2c0bb44 Alexey Dobriyan 2015-06-25  313  	free_page((unsigned long)page);
5ab82718 Linus Torvalds  2018-05-17  314  	return len;
c2c0bb44 Alexey Dobriyan 2015-06-25  315  }
c2c0bb44 Alexey Dobriyan 2015-06-25  316  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

end of thread, other threads:[~2018-06-20  9:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-19  6:07 regression: /proc/$pid/cmdline lacks trailing '\0' in 4.18-rc1 Michal Kubecek
2018-06-19  9:22 ` Michal Kubecek
2018-06-19 12:56   ` Michal Kubecek
2018-06-19 16:17     ` [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline kbuild test robot
2018-06-19 16:17   ` Michal Kubecek
2018-06-19 16:28     ` [PATCH v2] " Michal Kubecek
2018-06-19 17:14       ` Alexey Dobriyan
2018-06-19 21:56     ` [PATCH] " Linus Torvalds
2018-06-20  5:08       ` Michal Kubecek
2018-06-20  5:51         ` Linus Torvalds
2018-06-20  6:09           ` Michal Kubecek
2018-06-20  9:22     ` Dan Carpenter

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).