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

[Dan Carpenter <error27@gmail.com>]

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


--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html