proc: fix /proc/*/map_files lookup some more

PATCH Superseded Alexey Dobriyan 4 participants 6 messages Feb 21, 2018 Proc filesystem

lore

Share & tools

lore

Alexey Dobriyan

author

2018-02-21 18:44 UTC #0 lore raw

PATCH 1/1 proc: fix /proc/*/map_files lookup some more

I totally forgot that _parse_integer() accepts arbitrary amount of leading zeroes leading to the following:

		OK
	# readlink /proc/1/map_files/56427ecba000-56427eddc000
	/lib/systemd/systemd

		bogus
	# readlink /proc/1/map_files/00000000000056427ecba000-56427eddc000
	/lib/systemd/systemd
	# readlink /proc/1/map_files/56427ecba000-00000000000056427eddc000
	/lib/systemd/systemd

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/proc/base.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/proc/base.c +6 -1

@@ -1913,9 +1913,11 @@ static int dname_to_vma_addr(struct dentry *dentry,
 			     unsigned long *start, unsigned long *end)
 {
 	const char *str = dentry->d_name.name;
+	unsigned int len = dentry->d_name.len;
 	unsigned long long sval, eval;
-	unsigned int len;
 
+	if (len > 1 && *str == '0')
+		return -EINVAL;
 	len = _parse_integer(str, 16, &sval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

@@ -1927,6 +1929,9 @@ static int dname_to_vma_addr(struct dentry *dentry,
 		return -EINVAL;
 	str++;
 
+	len = strlen(str);
+	if (len > 1 && *str == '0')
+		return -EINVAL;
 	len = _parse_integer(str, 16, &eval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

Al Viro

2018-02-21 18:51 UTC #1 lore raw

fs/proc/base.c static int dname_to_vma_addr(struct dentry *dentry, · line 1934

Quoted 3 lines · Alexey Dobriyan

fs/proc/base.c +3 -0

+	len = strlen(str);
+	if (len > 1 && *str == '0')
+		return -EINVAL;

	if (s[0] == '0' && s[1])

please...

Alexey Dobriyan

author

2018-02-21 19:53 UTC #2 lore raw

PATCH v2 1/1 proc: fix /proc/*/map_files lookup some more

I totally forgot that _parse_integer() accepts arbitrary amount of leading zeroes leading to the following:

		OK
	# readlink /proc/1/map_files/56427ecba000-56427eddc000
	/lib/systemd/systemd

		bogus
	# readlink /proc/1/map_files/00000000000056427ecba000-56427eddc000
	/lib/systemd/systemd
	# readlink /proc/1/map_files/56427ecba000-00000000000056427eddc000
	/lib/systemd/systemd

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/proc/base.c |    4 ++++
 1 file changed, 4 insertions(+)

fs/proc/base.c +4 -0

@@ -1916,6 +1916,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 	unsigned long long sval, eval;
 	unsigned int len;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &sval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

@@ -1927,6 +1929,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 		return -EINVAL;
 	str++;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &eval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

Andrew Morton

2018-02-21 20:04 UTC #3 lore raw

Quoted 27 lines · On Wed, 21 Feb 2018 22:53:40 +0300 Alexey Dobriyan <[email protected]> wrote:

I totally forgot that _parse_integer() accepts arbitrary amount of leading zeroes leading to the following:

		OK
	# readlink /proc/1/map_files/56427ecba000-56427eddc000
	/lib/systemd/systemd

		bogus
	# readlink /proc/1/map_files/00000000000056427ecba000-56427eddc000
	/lib/systemd/systemd
	# readlink /proc/1/map_files/56427ecba000-00000000000056427eddc000
	/lib/systemd/systemd

...

fs/proc/base.c +2 -0

@@ -1916,6 +1916,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 	unsigned long long sval, eval;
 	unsigned int len;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &sval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

fs/proc/base.c static int dname_to_vma_addr(struct dentry *dentry, · line 1936

Quoted 8 lines

fs/proc/base.c +2 -0

@@ -1927,6 +1929,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 		return -EINVAL;
 	str++;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &eval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

I don't know this code and I'm all confused.

- why is the code designed to accept addresses of "0"?

- how do we know that the first digit of a VMA address will never be 0?

Cyrill Gorcunov

Reviewed-by

2018-02-21 20:29 UTC #4 lore raw

Quoted 4 lines · Andrew Morton

I don't know this code and I'm all confused.
- why is the code designed to accept addresses of "0"?

It was never designed to accept addresses of 0, it is rather a side effect of using sscanf in first place.

The address priting is done via

	len = snprintf(buf, sizeof(buf), "%lx-%lx", p->start, p->end);

Quoted 1 line

- how do we know that the first digit of a VMA address will never be 0?

It should not be, due to snprintf above.
---

Thanks a lot, Alexey!

Reviewed-by Cyrill Gorcunov <[email protected]>

Alexey Dobriyan

author

2018-02-21 21:02 UTC #5 lore raw

In reply to Andrew Morton

Quoted 40 lines · Andrew Morton

Quoted 27 lines · On Wed, 21 Feb 2018 22:53:40 +0300 Alexey Dobriyan <[email protected]> wrote:

I totally forgot that _parse_integer() accepts arbitrary amount of leading zeroes leading to the following:

		OK
	# readlink /proc/1/map_files/56427ecba000-56427eddc000
	/lib/systemd/systemd

		bogus
	# readlink /proc/1/map_files/00000000000056427ecba000-56427eddc000
	/lib/systemd/systemd
	# readlink /proc/1/map_files/56427ecba000-00000000000056427eddc000
	/lib/systemd/systemd

...

fs/proc/base.c +2 -0

@@ -1916,6 +1916,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 	unsigned long long sval, eval;
 	unsigned int len;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &sval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

fs/proc/base.c static int dname_to_vma_addr(struct dentry *dentry, · line 1936

Quoted 8 lines

fs/proc/base.c +2 -0

@@ -1927,6 +1929,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 		return -EINVAL;
 	str++;
 
+	if (str[0] == '0' && str[1])
+		return -EINVAL;
 	len = _parse_integer(str, 16, &eval);
 	if (len & KSTRTOX_OVERFLOW)
 		return -EINVAL;

I don't know this code and I'm all confused.

Quoted 1 line

- why is the code designed to accept addresses of "0"?

Now I'm confused.
Code rejects, say ,'07ff...-...' because printing with %lx-%lx would never produce leading zero.

Quoted 1 line

- how do we know that the first digit of a VMA address will never be 0?

Except when address is exactly 0 but this case is handled by looking at the second character.