fb: sm501fb off-by-1 sysfs store

Currently sm501fb_crtsrc_store() won't allow the routing to be changed via echos from userspace in to the sysfs file. The reason for this is that the strnicmp() for both heads uses a sizeof() for the string length, which ends up being strlen() + 1 (\0 in the normal case, but the echo gives a newline, which is where the issue occurs), this then causes a mismatch and subsequently bails with the -EINVAL.

In addition to this, the hardcoded lengths were then used for the store length that was returned, which ended up being erroneous and resulting in a write error. There's also no point in returning anything but the full length since it will -EINVAL out on a mismatch well before then anyways.

sizeof("string") is great for making sure you have space in your buffer, but rather less so for string comparisons :-)

Signed-off-by Paul Mundt <[email protected]>

--

 drivers/video/sm501fb.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/video/sm501fb.c +3 -4

index 02b290c..72b5358 100644
@@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	if (len < 1)
 		return -EINVAL;
 
-	if (strnicmp(buf, "crt", sizeof("crt")) == 0)
+	if (strnicmp(buf, "crt", 3) == 0)
 		head = HEAD_CRT;
-	else if (strnicmp(buf, "panel", sizeof("panel")) == 0)
+	else if (strnicmp(buf, "panel", 5) == 0)
 		head = HEAD_PANEL;
 	else
 		return -EINVAL;

@@ -1098,7 +1098,7 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	writel(ctrl, info->regs + SM501_DC_CRT_CONTROL);
 	sm501fb_sync_regs(info);
 
-	return (head == HEAD_CRT) ? 3 : 5;
+	return len;
 }
 
 /* Prepare the device_attr for registration with sysfs later */
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Currently sm501fb_crtsrc_store() won't allow the routing to be changed via echos from userspace in to the sysfs file. The reason for this is that the strnicmp() for both heads uses a sizeof() for the string length, which ends up being strlen() + 1 (\0 in the normal case, but the echo gives a newline, which is where the issue occurs), this then causes a mismatch and subsequently bails with the -EINVAL.

sizeof("string") is great for making sure you have space in your buffer, but rather less so for string comparisons :-)

drivers/video/sm501fb.c +0 -0

index 02b290c..72b5358 100644
@@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	if (len < 1)

Quoted 4 lines · Paul Mundt

drivers/video/sm501fb.c +0 -0

@@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	if (len < 1)
 		return -EINVAL;
 

drivers/video/sm501fb.c static ssize_t sm501fb_crtsrc_store(struct device *dev, · line 1082

Quoted 8 lines

drivers/video/sm501fb.c +2 -2

-	if (strnicmp(buf, "crt", sizeof("crt")) == 0)
+	if (strnicmp(buf, "crt", 3) == 0)
 		head = HEAD_CRT;
-	else if (strnicmp(buf, "panel", sizeof("panel")) == 0)
+	else if (strnicmp(buf, "panel", 5) == 0)
 		head = HEAD_PANEL;
 	else
 		return -EINVAL;

What about using just `strlen("string")' instead? ISTR gcc optimizes this to a constant. Not that it matters that much, as the actual string won't be duplicated.

Quoted 5 lines · Geert Uytterhoeven

Quoted 4 lines · Paul Mundt

drivers/video/sm501fb.c +0 -0

@@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
 	if (len < 1)
 		return -EINVAL;
 

Quoted 12 lines

drivers/video/sm501fb.c static ssize_t sm501fb_crtsrc_store(struct device *dev, · line 1082

Quoted 8 lines

drivers/video/sm501fb.c +2 -2

-	if (strnicmp(buf, "crt", sizeof("crt")) == 0)
+	if (strnicmp(buf, "crt", 3) == 0)
 		head = HEAD_CRT;
-	else if (strnicmp(buf, "panel", sizeof("panel")) == 0)
+	else if (strnicmp(buf, "panel", 5) == 0)
 		head = HEAD_PANEL;
 	else
 		return -EINVAL;

What about using just `strlen("string")' instead? ISTR gcc optimizes this to a constant. Not that it matters that much, as the actual string won't be duplicated.

I stuck with the constants since they were already referenced in the function, and there are enough other places in the kernel where they're used for these things. I didn't figure that 5 was a terribly high number to count to, but if there's expected difficulty, strlen() is of course an option.

Personally I find it less visually offensive with constants, but as long as the end number is the same, I'm not too concerned with how we get there. Feel free to change it if you wish.