Re: [PATCH v4 3/6] imx-drm: imx-ldb: Use snprintf()
On Wed, Feb 26, 2014 at 08:53:41PM -0300, Fabio Estevam wrote:
From: Fabio Estevam fabio.este...@freescale.com
Use snprintf() in order to fix the following static checker warning:
drivers/staging/imx-drm/imx-ldb.c:340 imx_ldb_get_clk() error: format string
overflow. buf_size: 16 length: 18
probably 18 is theory and not real life, but 16 is based on
theory as well.
Reported-by: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Fabio Estevam fabio.este...@freescale.com
---
Changes since v3:
- Fix Subject
- Also use snprintf in the other ocurrence
Changes since v2:
- Use snprintf as suggested by Russell
Changes since v1:
- None
drivers/staging/imx-drm/imx-ldb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/6] imx-drm: imx-ldb: Use snprintf()
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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/6] imx-drm: imx-ldb: Use snprintf()
On Thu, Feb 27, 2014 at 11:44:38PM +, 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.
Ick, I forgot that snprintf() takes into account the trailing \0, you
are right, this is safe.
Fabio, can you resend this please?
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/6] imx-drm: imx-ldb: Use snprintf()
On Thu, Feb 27, 2014 at 11:44:38PM +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
