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

[WANG Cong <xiyou.wangcong@gmail.com>]

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