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

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 <[email protected]>

---
kernel/time/clocksource.c | 19 ++++++++++---------

1 files changed, 10 insertions(+), 9 deletions(-)

kernel/time/clocksource.c +0 -0

index c8a9d13..5d5926f 100644
@@ -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) {

kernel/time/clocksource.c +1 -1

-	char *curr = buf;
+	ssize_t count = 0;

	spin_lock_irq(&clocksource_lock);

Quoted 54 lines · 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 <[email protected]>

---
kernel/time/clocksource.c | 19 ++++++++++---------

1 files changed, 10 insertions(+), 9 deletions(-)

kernel/time/clocksource.c +0 -0

index c8a9d13..5d5926f 100644
@@ -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) {

kernel/time/clocksource.c +1 -1

-	char *curr = buf;
+	ssize_t count = 0;

	spin_lock_irq(&clocksource_lock);

kernel/time/clocksource.c void clocksource_change_rating(struct clocksource *cs, int rating) · line 348

Quoted 2 lines

kernel/time/clocksource.c +1 -1

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

Quoted 8 lines · WANG Cong

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

Quoted 11 lines · on 2007-11-8 20:11 WANG Cong wrote:

Quoted 8 lines · WANG Cong

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.