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

Hello Jean,

Quoted 9 lines · Jean Delvare

sprintf isn't safe, use snprintf instead.

---
 tools/i2cbusses.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

tools/i2cbusses.c +0 -0

@@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
 

tools/i2cbusses.c struct i2c_adap *gather_i2c_busses(void) · line 223

Quoted 4 lines

tools/i2cbusses.c +1 -1

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

Quoted 4 lines

Quoted 4 lines

tools/i2cbusses.c +0 -0

@@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
 

Quoted 9 lines

tools/i2cbusses.c struct i2c_adap *gather_i2c_busses(void)

Quoted 4 lines

tools/i2cbusses.c +1 -1

 		/* 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(-)

tools/i2cbusses.c +1 -1

@@ -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));

Hello Jean,

Quoted 44 lines · Jean Delvare

Quoted 4 lines

Quoted 4 lines

tools/i2cbusses.c +0 -0

@@ -220,18 +220,18 @@ struct i2c_adap *gather_i2c_busses(void)
 

Quoted 9 lines

tools/i2cbusses.c struct i2c_adap *gather_i2c_busses(void)

Quoted 4 lines

tools/i2cbusses.c +1 -1

 		/* 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(-)

tools/i2cbusses.c +1 -1

@@ -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)
 

tools/i2cbusses.c struct i2c_adap *gather_i2c_busses(void) · line 227

Quoted 8 lines

tools/i2cbusses.c +5 -1

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