[PATCH] fb: sm501fb off-by-1 sysfs store

[PATCH] fb: sm501fb off-by-1 sysfs store

[Paul Mundt]

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 <lethal@linux-sh.org>

--

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

diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
index 02b290c..72b5358 100644
--- a/drivers/video/sm501fb.c
+++ b/drivers/video/sm501fb.c
@@ -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 */

Re: [PATCH] fb: sm501fb off-by-1 sysfs store

[Ben Dooks]

On Thu, Mar 01, 2007 at 05:24:56PM +0900, Paul Mundt wrote:
> 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 <lethal@linux-sh.org>
Acked-by: Ben Dooks <ben-linux@fluff.org>

> --
> 
>  drivers/video/sm501fb.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
> index 02b290c..72b5358 100644
> --- a/drivers/video/sm501fb.c
> +++ b/drivers/video/sm501fb.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [Linux-fbdev-devel] [PATCH] fb: sm501fb off-by-1 sysfs store

[Geert Uytterhoeven]

On Thu, 1 Mar 2007, Paul Mundt wrote:
> 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 :-)

> diff --git a/drivers/video/sm501fb.c b/drivers/video/sm501fb.c
> index 02b290c..72b5358 100644
> --- a/drivers/video/sm501fb.c
> +++ b/drivers/video/sm501fb.c
> @@ -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;

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.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [Linux-fbdev-devel] [PATCH] fb: sm501fb off-by-1 sysfs store

[Paul Mundt]

On Thu, Mar 01, 2007 at 02:25:55PM +0100, Geert Uytterhoeven wrote:
> On Thu, 1 Mar 2007, Paul Mundt wrote:
> > @@ -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;
> 
> 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.