mm/backing-dev.c: check user buffer length before copy data to the related user buffer.

PATCH In review Chen Gang 3 participants 7 messages Aug 20, 2013 – Aug 21, 2013

Subsystems (2)

Memory management MM misc

lore

Share & tools

lore

Chen Gang

author

2013-08-20 03:24 UTC #0 lore raw

PATCH 1/1 mm/backing-dev.c: check user buffer length before copy data to the related user buffer.

'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().

pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.

Signed-off-by Chen Gang <[email protected]>

---
 mm/backing-dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

mm/backing-dev.c +1 -1

index e04454c..2674671 100644
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}

Signature · 7 lines

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Jan Kara

Reviewed-by on v1 1/1

2013-08-20 15:28 UTC #1 lore raw

Quoted 7 lines · Chen Gang

'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.

Good catch. The patch looks good. You can add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 21 lines

Signed-off-by Chen Gang <[email protected]>

---
 mm/backing-dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

mm/backing-dev.c +1 -1

index e04454c..2674671 100644
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}

Signature · 1 line

1.7.7.6

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-08-21 02:29 UTC #2 lore raw

Quoted 11 lines · Jan Kara

Quoted 7 lines · Chen Gang
'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.
Good catch. The patch looks good. You can add:
Reviewed-by Jan Kara <[email protected]>

Thanks.

Quoted 22 lines

Honza

Quoted 21 lines

Signed-off-by Chen Gang <[email protected]>

---
 mm/backing-dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

mm/backing-dev.c +1 -1

index e04454c..2674671 100644
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}

Signature · 1 line

1.7.7.6

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2013-08-20 23:29 UTC #3 lore raw

In reply to Chen Gang

Quoted 12 lines · On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <[email protected]> wrote:

'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.
...

mm/backing-dev.c int pdflush_proc_obsolete(struct ctl_table *table, int write, · line 655

Quoted 8 lines

mm/backing-dev.c +1 -1

@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}

Well sort-of. If userspace opens /proc/sys/vm/nr_pdflush_threads and then does a series of one-byte reads, the kernel should return "0" on the first read, "\n" on the second and then EOF.

However this usually doesn't work in /proc anyway :(

akpm3:/tmp> cat /proc/sys/vm/max_map_count 65530
akpm3:/tmp> dd if=/proc/sys/vm/max_map_count of=foo bs=1 1+0 records in
1+0 records out
1 byte (1 B) copied, 0.00011963 s, 8.4 kB/s akpm3:/tmp> wc foo
0 1 1 foo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Chen Gang

author

2013-08-21 03:36 UTC #4 lore raw

Quoted 17 lines · Andrew Morton

Quoted 15 lines · On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <[email protected]> wrote:
'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.
...
mm/backing-dev.c +0 -0
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 

Quoted 10 lines

mm/backing-dev.c int pdflush_proc_obsolete(struct ctl_table *table, int write, · line 655
Quoted 5 lines
mm/backing-dev.c +1 -1
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}
Well sort-of. If userspace opens /proc/sys/vm/nr_pdflush_threads and then does a series of one-byte reads, the kernel should return "0" on the first read, "\n" on the second and then EOF.

Excuse me for my English, I guess your meaning is

"this patch is OK, but can be improvement"

Is it correct ?

Quoted 11 lines

However this usually doesn't work in /proc anyway :(
akpm3:/tmp> cat /proc/sys/vm/max_map_count 65530
akpm3:/tmp> dd if=/proc/sys/vm/max_map_count of=foo bs=1 1+0 records in
1+0 records out
1 byte (1 B) copied, 0.00011963 s, 8.4 kB/s akpm3:/tmp> wc foo
0 1 1 foo

Also excuse me for my English, I guess your meaning is:

for "dd if=/proc/sys/vm/* of=/tmp/foo bs=1",
need return "1 byte (1 B) copied", not 2 bytes (memory overflow), or 0 which meaningless.

If both of my guesses are correct, is the diff below better ?

----------------------------diff begin----------------------------------

mm/backing-dev.c +3 -3

index e04454c..d3be432 100644
@@ -647,19 +647,19 @@ EXPORT_SYMBOL(wait_iff_congested);
 int pdflush_proc_obsolete(struct ctl_table *table, int write,
 			void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	char kbuf[] = "0\n";
+	char kbuf[2] = {'0', '\n'};
 
 	if (*ppos) {
 		*lenp = 0;
 		return 0;
 	}
 
-	if (copy_to_user(buffer, kbuf, sizeof(kbuf)))
+	if (copy_to_user(buffer, kbuf, min(*lenp, sizeof(kbuf))))
 		return -EFAULT;
 	printk_once(KERN_WARNING "%s exported in /proc is scheduled for removal\n",
 			table->procname);
 
-	*lenp = 2;
+	*lenp = min(*lenp, sizeof(kbuf));
 	*ppos += *lenp;
 	return 2;
 }

----------------------------diff end------------------------------------

The diff above is not tested, if OK, I will send related patch after finish the related test.

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2013-08-21 03:46 UTC #5 lore raw

Quoted 29 lines · On Wed, 21 Aug 2013 11:35:04 +0800 Chen Gang <[email protected]> wrote:

Quoted 17 lines · Andrew Morton
Quoted 15 lines · On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <[email protected]> wrote:
'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.
...
mm/backing-dev.c +0 -0
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
Quoted 10 lines
mm/backing-dev.c EXPORT_SYMBOL(wait_iff_congested); · line 655
Quoted 5 lines
mm/backing-dev.c +1 -1
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}
Well sort-of. If userspace opens /proc/sys/vm/nr_pdflush_threads and then does a series of one-byte reads, the kernel should return "0" on the first read, "\n" on the second and then EOF.

Quoted 5 lines

Excuse me for my English, I guess your meaning is
"this patch is OK, but can be improvement"
Is it correct ?

Not really. I was pointing out that the patched code doesn't correctly implement read(1) behavior. But that is true of many other procfs files, so I suggest we not attempt to address the problem for this procfs file.

Chen Gang

author

2013-08-21 03:58 UTC #6 lore raw

Quoted 33 lines · Andrew Morton

Quoted 31 lines · On Wed, 21 Aug 2013 11:35:04 +0800 Chen Gang <[email protected]> wrote:
Quoted 17 lines · Andrew Morton
Quoted 15 lines · On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <[email protected]> wrote:
'*lenp' may be less than "sizeof(kbuf)", need check it before the next copy_to_user().
pdflush_proc_obsolete() is called by sysctl which 'procname' is "nr_pdflush_threads", if the user passes buffer length less than "sizeof(kbuf)", it will cause issue.
...
mm/backing-dev.c +0 -0
@@ -649,7 +649,7 @@ int pdflush_proc_obsolete(struct ctl_table *table, int write,
 {
 	char kbuf[] = "0\n";
 
Quoted 10 lines
mm/backing-dev.c int pdflush_proc_obsolete(struct ctl_table *table, int write, · line 655
Quoted 5 lines
mm/backing-dev.c +1 -1
-	if (*ppos) {
+	if (*ppos || *lenp < sizeof(kbuf)) {
 		*lenp = 0;
 		return 0;
 	}
Well sort-of. If userspace opens /proc/sys/vm/nr_pdflush_threads and then does a series of one-byte reads, the kernel should return "0" on the first read, "\n" on the second and then EOF.
Excuse me for my English, I guess your meaning is

Quoted 12 lines

Quoted 3 lines
"this patch is OK, but can be improvement"
Is it correct ?
Not really. I was pointing out that the patched code doesn't correctly implement read(1) behavior. But that is true of many other procfs files, so I suggest we not attempt to address the problem for this procfs file.

Hmm... does the fix below correctly implement read(1) behavior ?

----------------------------diff begin----------------------------------

mm/backing-dev.c +3 -3

index e04454c..d3be432 100644
@@ -647,19 +647,19 @@ EXPORT_SYMBOL(wait_iff_congested);
 int pdflush_proc_obsolete(struct ctl_table *table, int write,
 			void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-	char kbuf[] = "0\n";
+	char kbuf[2] = {'0', '\n'};
 
 	if (*ppos) {
 		*lenp = 0;
 		return 0;
 	}
 
-	if (copy_to_user(buffer, kbuf, sizeof(kbuf)))
+	if (copy_to_user(buffer, kbuf, min(*lenp, sizeof(kbuf))))
 		return -EFAULT;
 	printk_once(KERN_WARNING "%s exported in /proc is scheduled for removal\n",
 			table->procname);
 
-	*lenp = 2;
+	*lenp = min(*lenp, sizeof(kbuf));
 	*ppos += *lenp;
 	return 2;
 }

----------------------------diff end------------------------------------

The diff above is not tested, if OK, I will send related patch after finish the related test.

Thanks.

Signature · 7 lines

Chen Gang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>