diff mbox series

i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths

Message ID 20171031081604.18a9ca54@endymion
State Not Applicable
Headers show
Series i2c-tools: i2cbusses: Avoid buffer overflows in sysfs paths | expand

Commit Message

Jean Delvare Oct. 31, 2017, 7:16 a.m. UTC 
sprintf isn't safe, use snprintf instead.
---
 tools/i2cbusses.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Uwe Kleine-König Nov. 6, 2017, 12:04 p.m. UTC | #1 
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
Jean Delvare Nov. 8, 2017, 8:57 a.m. UTC | #2 
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;
 				}
Uwe Kleine-König Nov. 8, 2017, 9:29 a.m. UTC | #3 
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
Jean Delvare Nov. 8, 2017, 9:14 p.m. UTC | #4 
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.
diff mbox series

Patch

--- 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;
 				}