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;
Of course, that could change in the future, but is unlikely to change
significantly - and probably not much beyond two digit decimal.
So, the conversion from sprintf() to snprintf() is technically moot,
since it can only overflow if checks are removed elsewhere in this
code.
So really, this is just to shut the static checker up about something
that is a non-problem.
But there's another problem - and it's one of community process. The
reason these patches got raised is because another kernel maintainer
requested these errors to get fixed, so we're probably heading to the
situation where one maintainer wants them fixed and another doesn't...
I have no opinion either way. Personally, I'd have used snprintf()
here to start with so at least stack corruption can't happen, and
the worst that can happen is the string gets truncated, and the
following clk lookup fails, resulting in an error returned during
the driver probe.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
