* 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.