* i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths
@ 2017-10-31 7:16 Jean Delvare
2017-11-06 12:04 ` Uwe Kleine-König
0 siblings, 1 reply; 5+ messages in thread
From: Jean Delvare @ 2017-10-31 7:16 UTC (permalink / raw)
To: Linux I2C
sprintf isn't safe, use snprintf instead.
---
tools/i2cbusses.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--- a/tools/i2cbusses.c
+++ b/tools/i2cbusses.c
@@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
/* this should work for kernels 2.6.5 or higher and */
/* is preferred because is unambiguous */
- sprintf(n, "%s/%s/name", sysfs, de->d_name);
+ snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
f = fopen(n, "r");
/* this seems to work for ISA */
if(f == NULL) {
- sprintf(n, "%s/%s/device/name", sysfs, de->d_name);
+ snprintf(n, NAME_MAX, "%s/%s/device/name", sysfs, de->d_name);
f = fopen(n, "r");
}
/* non-ISA is much harder */
/* and this won't find the correct bus name if a driver
has more than one bus */
if(f == NULL) {
- sprintf(n, "%s/%s/device", sysfs, de->d_name);
+ snprintf(n, NAME_MAX, "%s/%s/device", sysfs, de->d_name);
if(!(ddir = opendir(n)))
continue;
while ((dde = readdir(ddir)) != NULL) {
@@ -240,8 +240,8 @@ struct i2c_adap *gather_i2c_busses(void)
if (!strcmp(dde->d_name, ".."))
continue;
if ((!strncmp(dde->d_name, "i2c-", 4))) {
- sprintf(n, "%s/%s/device/%s/name",
- sysfs, de->d_name, dde->d_name);
+ snprintf(n, NAME_MAX, "%s/%s/device/%s/name",
+ sysfs, de->d_name, dde->d_name);
if((f = fopen(n, "r")))
goto found;
}
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths
2017-10-31 7:16 i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths Jean Delvare
@ 2017-11-06 12:04 ` Uwe Kleine-König
2017-11-08 8:57 ` Jean Delvare
0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2017-11-06 12:04 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
Hello Jean,
On Tue, Oct 31, 2017 at 08:16:04AM +0100, Jean Delvare wrote:
> sprintf isn't safe, use snprintf instead.
> ---
> tools/i2cbusses.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> --- a/tools/i2cbusses.c
> +++ b/tools/i2cbusses.c
> @@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
>
> /* this should work for kernels 2.6.5 or higher and */
> /* is preferred because is unambiguous */
> - sprintf(n, "%s/%s/name", sysfs, de->d_name);
> + snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
OK, now instead of running in a buffer overflow in sprintf you might
call fopen with a partial (maybe unterminated?) filename. While this is
definitively better, you should check the return value of snprintf to be
completely safe here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths
2017-11-06 12:04 ` Uwe Kleine-König
@ 2017-11-08 8:57 ` Jean Delvare
2017-11-08 9:29 ` Uwe Kleine-König
0 siblings, 1 reply; 5+ messages in thread
From: Jean Delvare @ 2017-11-08 8:57 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Linux I2C
Hi Uwe,
Thanks for the review, very appreciated.
On Mon, 6 Nov 2017 13:04:26 +0100, Uwe Kleine-König wrote:
> Hello Jean,
>
> On Tue, Oct 31, 2017 at 08:16:04AM +0100, Jean Delvare wrote:
> > sprintf isn't safe, use snprintf instead.
> > ---
> > tools/i2cbusses.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > --- a/tools/i2cbusses.c
> > +++ b/tools/i2cbusses.c
> > @@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
> >
> > /* this should work for kernels 2.6.5 or higher and */
> > /* is preferred because is unambiguous */
> > - sprintf(n, "%s/%s/name", sysfs, de->d_name);
> > + snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
>
> OK, now instead of running in a buffer overflow in sprintf you might
> call fopen with a partial (maybe unterminated?) filename. While this is
> definitively better, you should check the return value of snprintf to be
> completely safe here.
To be honest, I never thought the buffer overflows could ever happen,
my motivation to fix them was to allow the code to build in OBS, where
FORTIFY_SOURCE is enabled. So I went for the most simple change that
made gcc happy.
That being said, I have no problem additionally checking the value
returned by snprintf. Something like this?
From: Jean Delvare
Subject: i2cbusses: Check the return value of snprintf
It's very unlikely that these paths will ever be truncated, but
better safe than sorry.
---
tools/i2cbusses.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
--- i2c-tools.orig/tools/i2cbusses.c 2017-11-02 16:17:50.698383029 +0100
+++ i2c-tools/tools/i2cbusses.c 2017-11-08 09:49:40.365339644 +0100
@@ -137,7 +137,7 @@ struct i2c_adap *gather_i2c_busses(void)
FILE *f;
char fstype[NAME_MAX], sysfs[NAME_MAX], n[NAME_MAX];
int foundsysfs = 0;
- int count=0;
+ int len, count = 0;
struct i2c_adap *adapters;
adapters = calloc(BUNCH, sizeof(struct i2c_adap));
@@ -220,18 +220,32 @@ struct i2c_adap *gather_i2c_busses(void)
/* this should work for kernels 2.6.5 or higher and */
/* is preferred because is unambiguous */
- snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
+ len = snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
+ if (len >= NAME_MAX) {
+ fprintf(stderr, "%s: path truncated\n", n);
+ continue;
+ }
f = fopen(n, "r");
/* this seems to work for ISA */
if(f == NULL) {
- snprintf(n, NAME_MAX, "%s/%s/device/name", sysfs, de->d_name);
+ len = snprintf(n, NAME_MAX, "%s/%s/device/name", sysfs,
+ de->d_name);
+ if (len >= NAME_MAX) {
+ fprintf(stderr, "%s: path truncated\n", n);
+ continue;
+ }
f = fopen(n, "r");
}
/* non-ISA is much harder */
/* and this won't find the correct bus name if a driver
has more than one bus */
if(f == NULL) {
- snprintf(n, NAME_MAX, "%s/%s/device", sysfs, de->d_name);
+ len = snprintf(n, NAME_MAX, "%s/%s/device", sysfs,
+ de->d_name);
+ if (len >= NAME_MAX) {
+ fprintf(stderr, "%s: path truncated\n", n);
+ continue;
+ }
if(!(ddir = opendir(n)))
continue;
while ((dde = readdir(ddir)) != NULL) {
@@ -240,8 +254,16 @@ struct i2c_adap *gather_i2c_busses(void)
if (!strcmp(dde->d_name, ".."))
continue;
if ((!strncmp(dde->d_name, "i2c-", 4))) {
- snprintf(n, NAME_MAX, "%s/%s/device/%s/name",
- sysfs, de->d_name, dde->d_name);
+ len = snprintf(n, NAME_MAX,
+ "%s/%s/device/%s/name",
+ sysfs, de->d_name,
+ dde->d_name);
+ if (len >= NAME_MAX) {
+ fprintf(stderr,
+ "%s: path truncated\n",
+ n);
+ continue;
+ }
if((f = fopen(n, "r")))
goto found;
}
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths
2017-11-08 8:57 ` Jean Delvare
@ 2017-11-08 9:29 ` Uwe Kleine-König
2017-11-08 21:14 ` Jean Delvare
0 siblings, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2017-11-08 9:29 UTC (permalink / raw)
To: Jean Delvare; +Cc: Linux I2C
Hello Jean,
On Wed, Nov 08, 2017 at 09:57:52AM +0100, Jean Delvare wrote:
> > > --- a/tools/i2cbusses.c
> > > +++ b/tools/i2cbusses.c
> > > @@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
> > >
> > > /* this should work for kernels 2.6.5 or higher and */
> > > /* is preferred because is unambiguous */
> > > - sprintf(n, "%s/%s/name", sysfs, de->d_name);
> > > + snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> >
> > OK, now instead of running in a buffer overflow in sprintf you might
> > call fopen with a partial (maybe unterminated?) filename. While this is
> > definitively better, you should check the return value of snprintf to be
> > completely safe here.
>
> To be honest, I never thought the buffer overflows could ever happen,
> my motivation to fix them was to allow the code to build in OBS, where
> FORTIFY_SOURCE is enabled. So I went for the most simple change that
> made gcc happy.
>
> That being said, I have no problem additionally checking the value
> returned by snprintf. Something like this?
>
> From: Jean Delvare
> Subject: i2cbusses: Check the return value of snprintf
>
> It's very unlikely that these paths will ever be truncated, but
> better safe than sorry.
> ---
> tools/i2cbusses.c | 34 ++++++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> --- i2c-tools.orig/tools/i2cbusses.c 2017-11-02 16:17:50.698383029 +0100
> +++ i2c-tools/tools/i2cbusses.c 2017-11-08 09:49:40.365339644 +0100
> @@ -137,7 +137,7 @@ struct i2c_adap *gather_i2c_busses(void)
> FILE *f;
> char fstype[NAME_MAX], sysfs[NAME_MAX], n[NAME_MAX];
> int foundsysfs = 0;
> - int count=0;
> + int len, count = 0;
> struct i2c_adap *adapters;
>
> adapters = calloc(BUNCH, sizeof(struct i2c_adap));
> @@ -220,18 +220,32 @@ struct i2c_adap *gather_i2c_busses(void)
>
> /* this should work for kernels 2.6.5 or higher and */
> /* is preferred because is unambiguous */
> - snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> + len = snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> + if (len >= NAME_MAX) {
> + fprintf(stderr, "%s: path truncated\n", n);
> + continue;
> + }
According to C99 snprintf et al return "the number of characters which
would have been written to the final string if enough space had been
available". Up to glibc 2.0.6 -1 was returned though if the output was
truncated. Does one still have to show consideration for a libc that
old? Hmm, probably not.
Then your change looks fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths
2017-11-08 9:29 ` Uwe Kleine-König
@ 2017-11-08 21:14 ` Jean Delvare
0 siblings, 0 replies; 5+ messages in thread
From: Jean Delvare @ 2017-11-08 21:14 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Linux I2C
On Wed, 8 Nov 2017 10:29:59 +0100, Uwe Kleine-König wrote:
> Hello Jean,
>
> On Wed, Nov 08, 2017 at 09:57:52AM +0100, Jean Delvare wrote:
> > > > --- a/tools/i2cbusses.c
> > > > +++ b/tools/i2cbusses.c
> > > > @@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
> > > >
> > > > /* this should work for kernels 2.6.5 or higher and */
> > > > /* is preferred because is unambiguous */
> > > > - sprintf(n, "%s/%s/name", sysfs, de->d_name);
> > > > + snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> > >
> > > OK, now instead of running in a buffer overflow in sprintf you might
> > > call fopen with a partial (maybe unterminated?) filename. While this is
> > > definitively better, you should check the return value of snprintf to be
> > > completely safe here.
> >
> > To be honest, I never thought the buffer overflows could ever happen,
> > my motivation to fix them was to allow the code to build in OBS, where
> > FORTIFY_SOURCE is enabled. So I went for the most simple change that
> > made gcc happy.
> >
> > That being said, I have no problem additionally checking the value
> > returned by snprintf. Something like this?
> >
> > From: Jean Delvare
> > Subject: i2cbusses: Check the return value of snprintf
> >
> > It's very unlikely that these paths will ever be truncated, but
> > better safe than sorry.
> > ---
> > tools/i2cbusses.c | 34 ++++++++++++++++++++++++++++------
> > 1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > --- i2c-tools.orig/tools/i2cbusses.c 2017-11-02 16:17:50.698383029 +0100
> > +++ i2c-tools/tools/i2cbusses.c 2017-11-08 09:49:40.365339644 +0100
> > @@ -137,7 +137,7 @@ struct i2c_adap *gather_i2c_busses(void)
> > FILE *f;
> > char fstype[NAME_MAX], sysfs[NAME_MAX], n[NAME_MAX];
> > int foundsysfs = 0;
> > - int count=0;
> > + int len, count = 0;
> > struct i2c_adap *adapters;
> >
> > adapters = calloc(BUNCH, sizeof(struct i2c_adap));
> > @@ -220,18 +220,32 @@ struct i2c_adap *gather_i2c_busses(void)
> >
> > /* this should work for kernels 2.6.5 or higher and */
> > /* is preferred because is unambiguous */
> > - snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> > + len = snprintf(n, NAME_MAX, "%s/%s/name", sysfs, de->d_name);
> > + if (len >= NAME_MAX) {
> > + fprintf(stderr, "%s: path truncated\n", n);
> > + continue;
> > + }
>
> According to C99 snprintf et al return "the number of characters which
> would have been written to the final string if enough space had been
> available". Up to glibc 2.0.6 -1 was returned though if the output was
> truncated. Does one still have to show consideration for a libc that
> old? Hmm, probably not.
I think we don't care, especially when nothing terribly bad would
happen then - simply back to the old behavior.
> Then your change looks fine.
Thanks for the review, I'll commit it.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-08 21:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-31 7:16 i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths Jean Delvare
2017-11-06 12:04 ` Uwe Kleine-König
2017-11-08 8:57 ` Jean Delvare
2017-11-08 9:29 ` Uwe Kleine-König
2017-11-08 21:14 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.