[PATCH v4 3/6] imx-drm: imx-ldb: Use snprintf()

[Dan Carpenter]

On Thu, Feb 27, 2014 at 11:44:38PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 27, 2014 at 02:54:43PM -0800, Greg KH wrote:
> > On Wed, Feb 26, 2014 at 08:53:41PM -0300, Fabio Estevam wrote:
> > > diff --git a/drivers/staging/imx-drm/imx-ldb.c b/drivers/staging/imx-drm/imx-ldb.c
> > > index abf8517..daa54df 100644
> > > --- a/drivers/staging/imx-drm/imx-ldb.c
> > > +++ b/drivers/staging/imx-drm/imx-ldb.c
> > > @@ -334,12 +334,12 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
> > >  {
> > >  	char clkname[16];
> > >  
> > > -	sprintf(clkname, "di%d", chno);
> > > +	snprintf(clkname, sizeof(clkname), "di%d", chno);
> > 
> > Are you sure this can not overflow as well?  Strings in C are nasty...
> 
> Can you indicate how this would overflow?
> 
>  * snprintf - Format a string and place it in a buffer
> ...
>  *
>  * The return value is the number of characters which would be
>  * generated for the given input, excluding the trailing null,
>  * as per ISO C99.  If the return is greater than or equal to
>  * @size, the resulting string is truncated.
> 
> Now, there's several layers of protection here.  The first obvious one
> is using snprintf() instead of sprintf() which wouldn't know the buffer
> size.
> 
> The second one is something that the static checker can't know, and
> that is for existing uses, chno is limited to zero or one:
> 
>                 ret = of_property_read_u32(child, "reg", &i);
>                 if (ret || i < 0 || i > 1)
>                         return -EINVAL;
> 

If you have the cross function database built then Smatch wouldn't have
complained.  But this driver is outside of my normal build so I didn't
have that.

Of course, my first impression was that this wasn't a real bug.  But
these things are easy to solve and people who don't use snprintf()
should be more careful about picking buffer sizes so I don't care about
harrassing people with false positives.  ;)

If the code were old and outside of staging then I wouldn't have
mentioned the warning.

regards,
dan carpenter