fbdev: fix snprintf() limit in show_bl_curve()

Started by Dan Carpenter on Aug 21, 2015 · 4 messages from 2 authors · Last reply Aug 21, 2015

Subsystems (3)

Maintainers (3)

STALE4101d

open the first message on its own page

Message details and related content
The limit should be "PAGE_SIZE - len" instead of PAGE_SIZE.

Signed-off-by: Dan Carpenter <redacted>
diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index 60c3f0a..827098d 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -485,7 +485,7 @@ static ssize_t show_bl_curve(struct device *device,
 
 	mutex_lock(&fb_info->bl_curve_mutex);
 	for (i = 0; i < FB_BACKLIGHT_LEVELS; i += 8)
-		len += snprintf(&buf[len], PAGE_SIZE, "%8ph\n",
+		len += snprintf(&buf[len], PAGE_SIZE - len, "%8ph\n",
 				fb_info->bl_curve + i);
 	mutex_unlock(&fb_info->bl_curve_mutex);
 
Message details and related content
On Fri, 2015-08-21 at 11:53 +0300, Dan Carpenter wrote:
The limit should be "PAGE_SIZE - len" instead of PAGE_SIZE.
Besides that was in the original code, the problem still might happen
when FB_BACKLIGHT_LEVELS is set to 171+ since snprintf() returns
desired length. I suppose you would change this to check len on each
iteration or change to scnprintf() if I get it correct.

 
quoted hunk
diff --git a/drivers/video/fbdev/core/fbsysfs.c 
b/drivers/video/fbdev/core/fbsysfs.c
index 60c3f0a..827098d 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -485,7 +485,7 @@ static ssize_t show_bl_curve(struct device 
*device,
 
 	mutex_lock(&fb_info->bl_curve_mutex);
 	for (i = 0; i < FB_BACKLIGHT_LEVELS; i += 8)
-		len += snprintf(&buf[len], PAGE_SIZE, "%8ph\n",
+		len += snprintf(&buf[len], PAGE_SIZE - len, 
"%8ph\n",
 				fb_info->bl_curve + i);
 	mutex_unlock(&fb_info->bl_curve_mutex);
 
-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy
Message details and related content
On Fri, Aug 21, 2015 at 12:22:03PM +0300, Andy Shevchenko wrote:
On Fri, 2015-08-21 at 11:53 +0300, Dan Carpenter wrote:
quoted
The limit should be "PAGE_SIZE - len" instead of PAGE_SIZE.
Besides that was in the original code, the problem still might happen
when FB_BACKLIGHT_LEVELS is set to 171+ since snprintf() returns
desired length. I suppose you would change this to check len on each
iteration or change to scnprintf() if I get it correct.
Yeah, you're right.

If you pass a negative size to snprintf() it won't overflow, but it will
print an error.  I will send a v2 and change it to scnprintf().  I don't
think we will have 171 levels so it's not worth adding the check?

regards,
dan carpenter
Message details and related content
On Fri, 2015-08-21 at 12:53 +0300, Dan Carpenter wrote:
On Fri, Aug 21, 2015 at 12:22:03PM +0300, Andy Shevchenko wrote:
quoted
On Fri, 2015-08-21 at 11:53 +0300, Dan Carpenter wrote:
quoted
The limit should be "PAGE_SIZE - len" instead of PAGE_SIZE.
Besides that was in the original code, the problem still might 
happen
when FB_BACKLIGHT_LEVELS is set to 171+ since snprintf() returns
desired length. I suppose you would change this to check len on 
each
iteration or change to scnprintf() if I get it correct.
Yeah, you're right.

If you pass a negative size to snprintf() it won't overflow, but it 
will
print an error.  I will send a v2 and change it to scnprintf().  I 
don't
think we will have 171 levels so it's not worth adding the check?
I think there is no need to check for that. Especially it might be
changed in the future (if, for example, they decide to have 16 levels
per line instead of 8).
regards,
dan carpenter
-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help