[PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[Miao Xie]

Hi,every one.
   I found that there is a buffer overflow problem in the following code.

Version:	2.6.24-rc2,
File:		kernel/time/clocksource.c:417-432
--------------------------------------------------------------------
static ssize_t
sysfs_show_available_clocksources(struct sys_device *dev, char *buf)
{
	struct clocksource *src;
	char *curr = buf;

	spin_lock_irq(&clocksource_lock);
	list_for_each_entry(src, &clocksource_list, list) {
		curr += sprintf(curr, "%s ", src->name);
	}
	spin_unlock_irq(&clocksource_lock);

	curr += sprintf(curr, "\n");

	return curr - buf;
}
-----------------------------------------------------------------------

sysfs_show_current_clocksources() also has the same problem though in practice
the size of current clocksource's name won't exceed PAGE_SIZE.

I fix the bug by using snprintf according to the specification of the kernel
(Version:2.6.24-rc2,File:Documentation/filesystems/sysfs.txt)

Fix sysfs_show_available_clocksources() and sysfs_show_current_clocksources()
buffer overflow problem with snprintf().

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>

---
  kernel/time/clocksource.c |   19 ++++++++++---------
  1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index c8a9d13..5d5926f 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -342,15 +342,13 @@ void clocksource_change_rating(struct clocksource *cs, int rating)
  static ssize_t
  sysfs_show_current_clocksources(struct sys_device *dev, char *buf)
  {
-	char *curr = buf;
+	ssize_t count = 0;

  	spin_lock_irq(&clocksource_lock);
-	curr += sprintf(curr, "%s ", curr_clocksource->name);
+	count = snprintf(buf, PAGE_SIZE, "%s\n", curr_clocksource->name);
  	spin_unlock_irq(&clocksource_lock);

-	curr += sprintf(curr, "\n");
-
-	return curr - buf;
+	return count;
  }

  /**
@@ -418,17 +416,20 @@ static ssize_t
  sysfs_show_available_clocksources(struct sys_device *dev, char *buf)
  {
  	struct clocksource *src;
-	char *curr = buf;
+	ssize_t count = 0;

  	spin_lock_irq(&clocksource_lock);
  	list_for_each_entry(src, &clocksource_list, list) {
-		curr += sprintf(curr, "%s ", src->name);
+		count += snprintf(buf + count,
+				  max((ssize_t)PAGE_SIZE - count, (ssize_t)0),
+				  "%s ", src->name);
  	}
  	spin_unlock_irq(&clocksource_lock);

-	curr += sprintf(curr, "\n");
+	count += snprintf(buf + count,
+			  max((ssize_t)PAGE_SIZE - count, (ssize_t)0), "\n");

-	return curr - buf;
+	return count;
  }

  /*
-- 
1.5.3

Re: [PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[WANG Cong]

On Thu, Nov 08, 2007 at 06:53:40PM +0800, Miao Xie wrote:
>Hi,every one.
>  I found that there is a buffer overflow problem in the following code.
>
>Version:	2.6.24-rc2,
>File:		kernel/time/clocksource.c:417-432
>--------------------------------------------------------------------
>static ssize_t
>sysfs_show_available_clocksources(struct sys_device *dev, char *buf)
>{
>	struct clocksource *src;
>	char *curr = buf;
>
>	spin_lock_irq(&clocksource_lock);
>	list_for_each_entry(src, &clocksource_list, list) {
>		curr += sprintf(curr, "%s ", src->name);
>	}
>	spin_unlock_irq(&clocksource_lock);
>
>	curr += sprintf(curr, "\n");
>
>	return curr - buf;
>}
>-----------------------------------------------------------------------
>
>sysfs_show_current_clocksources() also has the same problem though in 
>practice
>the size of current clocksource's name won't exceed PAGE_SIZE.
>
>I fix the bug by using snprintf according to the specification of the kernel
>(Version:2.6.24-rc2,File:Documentation/filesystems/sysfs.txt)
>
>Fix sysfs_show_available_clocksources() and 
>sysfs_show_current_clocksources()
>buffer overflow problem with snprintf().
>
>Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>
>---
> kernel/time/clocksource.c |   19 ++++++++++---------
> 1 files changed, 10 insertions(+), 9 deletions(-)
>
>diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
>index c8a9d13..5d5926f 100644
>--- a/kernel/time/clocksource.c
>+++ b/kernel/time/clocksource.c
>@@ -342,15 +342,13 @@ void clocksource_change_rating(struct clocksource 
>*cs, int rating)
> static ssize_t
> sysfs_show_current_clocksources(struct sys_device *dev, char *buf)
> {
>-	char *curr = buf;
>+	ssize_t count = 0;
>
> 	spin_lock_irq(&clocksource_lock);
>-	curr += sprintf(curr, "%s ", curr_clocksource->name);
>+	count = snprintf(buf, PAGE_SIZE, "%s\n", curr_clocksource->name);

Yes, snprintf is safer than sprintf. But here, the 'count' will be
mis-pointed when snprintf returns no less than PAGE_SIZE (what you called
overflow). So you may also need:

	if (unlikely(count >= PAGE_SIZE))
		count = PAGE_SIZE - 1;

Just a simple guess. ;)

Re: [PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[WANG Cong]

On Thu, Nov 08, 2007 at 07:47:41PM +0800, WANG Cong wrote:
>On Thu, Nov 08, 2007 at 06:53:40PM +0800, Miao Xie wrote:
>>Hi,every one.
>>  I found that there is a buffer overflow problem in the following code.
>>
>>Version:	2.6.24-rc2,
>>File:		kernel/time/clocksource.c:417-432
>>--------------------------------------------------------------------
>>static ssize_t
>>sysfs_show_available_clocksources(struct sys_device *dev, char *buf)
>>{
>>	struct clocksource *src;
>>	char *curr = buf;
>>
>>	spin_lock_irq(&clocksource_lock);
>>	list_for_each_entry(src, &clocksource_list, list) {
>>		curr += sprintf(curr, "%s ", src->name);
>>	}
>>	spin_unlock_irq(&clocksource_lock);
>>
>>	curr += sprintf(curr, "\n");
>>
>>	return curr - buf;
>>}
>>-----------------------------------------------------------------------
>>
>>sysfs_show_current_clocksources() also has the same problem though in 
>>practice
>>the size of current clocksource's name won't exceed PAGE_SIZE.
>>
>>I fix the bug by using snprintf according to the specification of the kernel
>>(Version:2.6.24-rc2,File:Documentation/filesystems/sysfs.txt)
>>
>>Fix sysfs_show_available_clocksources() and 
>>sysfs_show_current_clocksources()
>>buffer overflow problem with snprintf().
>>
>>Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>>
>>---
>> kernel/time/clocksource.c |   19 ++++++++++---------
>> 1 files changed, 10 insertions(+), 9 deletions(-)
>>
>>diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
>>index c8a9d13..5d5926f 100644
>>--- a/kernel/time/clocksource.c
>>+++ b/kernel/time/clocksource.c
>>@@ -342,15 +342,13 @@ void clocksource_change_rating(struct clocksource 
>>*cs, int rating)
>> static ssize_t
>> sysfs_show_current_clocksources(struct sys_device *dev, char *buf)
>> {
>>-	char *curr = buf;
>>+	ssize_t count = 0;
>>
>> 	spin_lock_irq(&clocksource_lock);
>>-	curr += sprintf(curr, "%s ", curr_clocksource->name);
>>+	count = snprintf(buf, PAGE_SIZE, "%s\n", curr_clocksource->name);
>
>Yes, snprintf is safer than sprintf. But here, the 'count' will be
>mis-pointed when snprintf returns no less than PAGE_SIZE (what you called
>overflow). So you may also need:
>
>	if (unlikely(count >= PAGE_SIZE))
>		count = PAGE_SIZE - 1;
>
>Just a simple guess. ;)

Or try scnprintf. ;)

Re: [PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[Miao Xie]

on 2007-11-8 20:11 WANG Cong wrote:
> On Thu, Nov 08, 2007 at 07:47:41PM +0800, WANG Cong wrote:
>> Yes, snprintf is safer than sprintf. But here, the 'count' will be
>> mis-pointed when snprintf returns no less than PAGE_SIZE (what you called
>> overflow). So you may also need:
>>
>> 	if (unlikely(count >= PAGE_SIZE))
>> 		count = PAGE_SIZE - 1;
>>
>> Just a simple guess. ;)
> 
> Or try scnprintf. ;)

We have discussed this problem. We think that it is better to return the return
value of kernel directly because this is the specification of the sysfs.

   (Version:2.6.24-rc2,File:Documentation/filesystems/sysfs.txt:198-201):
   198 - show() methods should return the number of bytes printed into the
   199   buffer. This is the return value of snprintf().
   200
   201 - show() should always use snprintf().

And the function which calls the show() methods uses BUG_ON() to check the
return value. If the return value is too big,it means something wrong.

If we use scnprintf, we may not know whether the resulting string is truncated
or not. Maybe A big bug is ignored.

Re: [PATCH] time: fix sysfs_show_{available,current}_clocksources() buffer overflow problem

[WANG Cong]

On Sun, Nov 11, 2007 at 11:29:59AM +0800, Miao Xie wrote:
>on 2007-11-8 20:11 WANG Cong wrote:
>>On Thu, Nov 08, 2007 at 07:47:41PM +0800, WANG Cong wrote:
>>>Yes, snprintf is safer than sprintf. But here, the 'count' will be
>>>mis-pointed when snprintf returns no less than PAGE_SIZE (what you called
>>>overflow). So you may also need:
>>>
>>>	if (unlikely(count >= PAGE_SIZE))
>>>		count = PAGE_SIZE - 1;
>>>
>>>Just a simple guess. ;)
>>
>>Or try scnprintf. ;)
>
>We have discussed this problem. We think that it is better to return the 
>return
>value of kernel directly because this is the specification of the sysfs.
>
>  (Version:2.6.24-rc2,File:Documentation/filesystems/sysfs.txt:198-201):
>  198 - show() methods should return the number of bytes printed into the
>  199   buffer. This is the return value of snprintf().
>  200
>  201 - show() should always use snprintf().
>
>And the function which calls the show() methods uses BUG_ON() to check the
>return value. If the return value is too big,it means something wrong.
>
>If we use scnprintf, we may not know whether the resulting string is 
>truncated
>or not. Maybe A big bug is ignored.
>

Well, i know little about sysfs. So it was just a hint.

Anyway, thanks for your input!