[patch] prism54: call BUG_ON() earlier

[patch] prism54: call BUG_ON() earlier

[Dan Carpenter]

This test is off by one because strlen() doesn't include the NULL
terminator.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/net/wireless/prism54/isl_ioctl.c b/drivers/net/wireless/prism54/isl_ioctl.c
index 8d1190c..1051268 100644
--- a/drivers/net/wireless/prism54/isl_ioctl.c
+++ b/drivers/net/wireless/prism54/isl_ioctl.c
@@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
 	memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
 	if (!memptr)
 		return;
-	BUG_ON(n > IW_CUSTOM_MAX);
+	BUG_ON(n >= IW_CUSTOM_MAX);
 	wrqu.data.pointer = memptr;
 	wrqu.data.length = n;
 	strcpy(memptr, str);

Re: [patch] prism54: call BUG_ON() earlier

[Luis R. Rodriguez]

On Sat, Jul 10, 2010 at 1:53 AM, Dan Carpenter <error27@gmail.com> wrote:
> This test is off by one because strlen() doesn't include the NULL
> terminator.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/drivers/net/wireless/prism54/isl_ioctl.c b/drivers/net/wireless/prism54/isl_ioctl.c
> index 8d1190c..1051268 100644
> --- a/drivers/net/wireless/prism54/isl_ioctl.c
> +++ b/drivers/net/wireless/prism54/isl_ioctl.c
> @@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
>        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
>        if (!memptr)
>                return;
> -       BUG_ON(n > IW_CUSTOM_MAX);
> +       BUG_ON(n >= IW_CUSTOM_MAX);
>        wrqu.data.pointer = memptr;
>        wrqu.data.length = n;
>        strcpy(memptr, str);
>

send_simple_event() never passes a NULL terminated string though. What
does this fix today? If nothing then better leave as-is.

  Luis

Re: [patch] prism54: call BUG_ON() earlier

[Dan Carpenter]

On Mon, Jul 12, 2010 at 10:06:55AM -0700, Luis R. Rodriguez wrote:
> On Sat, Jul 10, 2010 at 1:53 AM, Dan Carpenter <error27@gmail.com> wrote:
> > This test is off by one because strlen() doesn't include the NULL
> > terminator.
> >
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> >
> > diff --git a/drivers/net/wireless/prism54/isl_ioctl.c b/drivers/net/wireless/prism54/isl_ioctl.c
> > index 8d1190c..1051268 100644
> > --- a/drivers/net/wireless/prism54/isl_ioctl.c
> > +++ b/drivers/net/wireless/prism54/isl_ioctl.c
> > @@ -2067,7 +2067,7 @@ send_simple_event(islpci_private *priv, const char *str)
> >        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
> >        if (!memptr)
> >                return;
> > -       BUG_ON(n > IW_CUSTOM_MAX);
> > +       BUG_ON(n >= IW_CUSTOM_MAX);
> >        wrqu.data.pointer = memptr;
> >        wrqu.data.length = n;
> >        strcpy(memptr, str);
> >
> 
> send_simple_event() never passes a NULL terminated string though. What
> does this fix today? If nothing then better leave as-is.
> 
>   Luis

It doesn't fix any bugs in the current code, but it's a necessary clean
up.

        memptr = kmalloc(IW_CUSTOM_MAX, GFP_KERNEL);
	                 ^^^^^^^^^^^^^
	This is the size of memptr.

        if (!memptr)
                return;
        BUG_ON(n > IW_CUSTOM_MAX);

	^^^^^^^^^^^^^^^^^^^^^^^^^^
	This is an off-by-one check.

        wrqu.data.pointer = memptr;
        wrqu.data.length = n;
        strcpy(memptr, str);
	^^^^^^^^^^^^^^^^^^^^

	This would be a silent memory corruption.

In the current code we only use short event strings so the check isn't
needed.  But we should either correct the check or remove it.

regards,
dan carpenter

Re: [patch] prism54: call BUG_ON() earlier

[Dan Carpenter]

Of course the main reasons is that it silences a static checkers
warning.  :P

drivers/net/wireless/prism54/isl_ioctl.c +2073 send_simple_event(12)
	error: strcpy() 'str' too large for 'memptr' (257 vs 256)

regards,
dan carpenter