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

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

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


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/backing-dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index e04454c..2674671 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -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;
 	}
-- 
1.7.7.6

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

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

[Jan Kara]

On Tue 20-08-13 11:23:24, Chen Gang 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.
> 
  Good catch. The patch looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  mm/backing-dev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index e04454c..2674671 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -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;
>  	}
> -- 
> 1.7.7.6
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

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

[Andrew Morton]

On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <gang.chen@asianux.com> 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.
> 
> ...
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -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 majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Chen Gang]

On 08/20/2013 11:28 PM, Jan Kara wrote:
> On Tue 20-08-13 11:23:24, Chen Gang 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.
>>
>   Good catch. The patch looks good. You can add:
> Reviewed-by: Jan Kara <jack@suse.cz>
> 

Thanks.

> 								Honza
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  mm/backing-dev.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> index e04454c..2674671 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -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;
>>  	}
>> -- 
>> 1.7.7.6


-- 
Chen Gang

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

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

[Chen Gang]

On 08/21/2013 07:29 AM, Andrew Morton wrote:
> On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <gang.chen@asianux.com> 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.
>>
>> ...
>>
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -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.
> 

Excuse me for my English, I guess your meaning is

  "this patch is OK, but can be improvement"

Is it correct ?

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

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index e04454c..d3be432 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -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.
-- 
Chen Gang

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

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

[Andrew Morton]

On Wed, 21 Aug 2013 11:35:04 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> On 08/21/2013 07:29 AM, Andrew Morton wrote:
> > On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <gang.chen@asianux.com> 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.
> >>
> >> ...
> >>
> >> --- a/mm/backing-dev.c
> >> +++ b/mm/backing-dev.c
> >> @@ -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.
> > 
> 
> 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.


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

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

[Chen Gang]

On 08/21/2013 11:45 AM, Andrew Morton wrote:
> On Wed, 21 Aug 2013 11:35:04 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>> On 08/21/2013 07:29 AM, Andrew Morton wrote:
>>> On Tue, 20 Aug 2013 11:23:24 +0800 Chen Gang <gang.chen@asianux.com> 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.
>>>>
>>>> ...
>>>>
>>>> --- a/mm/backing-dev.c
>>>> +++ b/mm/backing-dev.c
>>>> @@ -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.
>>>
>>
>> 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.
> 
> 
> 
> 

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

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

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index e04454c..d3be432 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -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.
-- 
Chen Gang

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