udev - read long lines from config files overflow fix

PATCH Applied Arun Bhanu 3 participants 5 messages Sep 4, 2004 – Sep 10, 2004

Share & tools

Arun Bhanu

author

2004-09-04 17:08 UTC #0 lore raw

PATCH 1/1 udev - read long lines from config files overflow fix

A line containing more than 255 characters in any of the config files - *.rules, *.permissions or udev.conf - causes udevstart to segfault while reading them. To reproduce, put a comment line longer than 255 characters in any of the above files.

The following patch against udev-030 fixes it.

                --Arun

Signed-off-by Arun Bhanu <[email protected]>

klibc_fixups.c +5 -3

diff -Nru a/klibc_fixups.c b/klibc_fixups.c
@@ -41,7 +41,7 @@
 static unsigned long get_id_by_name(const char *uname, const char *dbfile)
 {
 	unsigned long id = -1;
-	char line[255];
+	char line[MAXLINE_LEN+1];
 	char *buf;
 	size_t bufsize;
 	size_t cur;

@@ -50,6 +50,7 @@
 	char *name;
 	char *idstr;
 	char *tail;
+	size_t linelen;
 
 	if (file_map(dbfile, &buf, &bufsize) = 0) {
 		dbg("reading '%s' as db file", dbfile);

@@ -64,8 +65,9 @@
 	while (1) {
 		count = buf_get_line(buf, bufsize, cur);
 
-		strncpy(line, buf + cur, count);
-		line[count] = '\0';
+		linelen = (count > MAXLINE_LEN) ? MAXLINE_LEN : count;
+		strncpy(line, buf + cur, linelen);
+		line[linelen] = '\0';
 		pos = line;
 
 		cur += count+1;

namedev_parse.c +10 -6

diff -Nru a/namedev_parse.c b/namedev_parse.c
@@ -144,7 +144,7 @@
 
 static int namedev_parse_rules(char *filename)
 {
-	char line[255];
+	char line[MAXLINE_LEN+1];
 	int lineno;
 	char *temp;
 	char *temp2;

@@ -157,6 +157,7 @@
 	int program_given = 0;
 	int retval = 0;
 	struct config_device dev;
+	size_t linelen;
 
 	if (file_map(filename, &buf, &bufsize) = 0) {
 		dbg("reading '%s' as rules file", filename);

@@ -171,8 +172,9 @@
 	while (1) {
 		count = buf_get_line(buf, bufsize, cur);
 
-		strncpy(line, buf + cur, count);
-		line[count] = '\0';
+		linelen = (count > MAXLINE_LEN) ? MAXLINE_LEN : count;
+		strncpy(line, buf + cur, linelen);
+		line[linelen] = '\0';
 		temp = line;
 		lineno++;

@@ -324,7 +326,7 @@
 
 static int namedev_parse_permissions(char *filename)
 {
-	char line[255];
+	char line[MAXLINE_LEN+1];
 	char *temp;
 	char *temp2;
 	char *buf;

@@ -333,6 +335,7 @@
 	size_t count;
 	int retval = 0;
 	struct perm_device dev;
+	size_t linelen;
 
 	if (file_map(filename, &buf, &bufsize) = 0) {
 		dbg("reading '%s' as permissions file", filename);

@@ -346,8 +349,9 @@
 	while (1) {
 		count = buf_get_line(buf, bufsize, cur);
 
-		strncpy(line, buf + cur, count);
-		line[count] = '\0';
+		linelen = (count > MAXLINE_LEN) ? MAXLINE_LEN : count;
+		strncpy(line, buf + cur, linelen);
+		line[linelen] = '\0';
 		temp = line;
 
 		cur += count+1;

udev.h +1 -0

diff -Nru a/udev.h b/udev.h
@@ -36,6 +36,7 @@
 #define ACTION_SIZE			30
 #define DEVPATH_SIZE			255
 #define SUBSYSTEM_SIZE			30
+#define MAXLINE_LEN			255
 
 /* length of public data */
 #define UDEVICE_LEN (offsetof(struct udevice, bus_id))

udev_config.c +5 -3

diff -Nru a/udev_config.c b/udev_config.c
@@ -127,7 +127,7 @@
 
 static int parse_config_file(void)
 {
-	char line[255];
+	char line[MAXLINE_LEN+1];
 	char *temp;
 	char *variable;
 	char *value;

@@ -137,6 +137,7 @@
 	size_t count;
 	int lineno;
 	int retval = 0;
+	size_t linelen;
 
 	if (file_map(udev_config_filename, &buf, &bufsize) = 0) {
 		dbg("reading '%s' as config file", udev_config_filename);

@@ -151,8 +152,9 @@
 	while (1) {
 		count = buf_get_line(buf, bufsize, cur);
 
-		strncpy(line, buf + cur, count);
-		line[count] = '\0';
+		linelen = (count > MAXLINE_LEN) ? MAXLINE_LEN : count;
+		strncpy(line, buf + cur, linelen);
+		line[linelen] = '\0';
 		temp = line;
 		lineno++;

-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_idP47&alloc_id808&op=click
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Kay Sievers

2004-09-04 21:12 UTC #1 lore raw

Quoted 6 lines · Arun Bhanu

A line containing more than 255 characters in any of the config files - *.rules, *.permissions or udev.conf - causes udevstart to segfault while reading them. To reproduce, put a comment line longer than 255 characters in any of the above files.
The following patch against udev-030 fixes it.

Cool, a real bug :)
Thanks, for the patch. I think it would be better to skip lenghth exceeding lines instead of cutting it and continue. While looking at it I restructured the buffer reading logic a bit and fixed another stupid bug.

o Lines with a exceeding length are skipped.

o we now skip empty lines, comment lines and leading whitespace before we copy over our line string to parse.

o Fix for another bug. If we don't find any valuable key in a line, we need to skip this line completely and not assume that this is a ignore rule.

o Send line parsing errors to log, as nobody will ever notice it without recompilation of udev

Thanks,
Kay

===== klibc_fixups.c 1.10 vs edited =====

klibc_fixups.c +9 -8

@@ -41,8 +41,9 @@
 static unsigned long get_id_by_name(const char *uname, const char *dbfile)
 {
 	unsigned long id = -1;
-	char line[255];
+	char line[LINE_SIZE];
 	char *buf;
+	char *bufline;
 	size_t bufsize;
 	size_t cur;
 	size_t count;

@@ -59,18 +60,18 @@ static unsigned long get_id_by_name(cons
 	}
 
 	/* loop through the whole file */
-
 	cur = 0;
-	while (1) {
+	while (cur < bufsize) {
 		count = buf_get_line(buf, bufsize, cur);
+		bufline = &buf[cur];
+		cur += count+1;
+
+		if (count >= LINE_SIZE)
+			continue;
 
-		strncpy(line, buf + cur, count);
+		strncpy(line, bufline, count);
 		line[count] = '\0';
 		pos = line;
-
-		cur += count+1;
-		if (cur > bufsize)
-			break;
 
 		/* get name */
 		name = strsep(&pos, ":");

===== namedev_parse.c 1.34 vs edited =====

namedev_parse.c +71 ~~-43~~

267 lines not on this page · raw message

===== udev.h 1.58 vs edited =====

udev.h +2 -0

@@ -37,6 +37,8 @@
 #define DEVPATH_SIZE			255
 #define SUBSYSTEM_SIZE			30
 
+#define LINE_SIZE			256
+
 /* length of public data */
 #define UDEVICE_LEN (offsetof(struct udevice, bus_id))

Arun Bhanu

author

2004-09-05 18:28 UTC #2 lore raw

In reply to Arun Bhanu

Hi Kay,

Quoted 4 lines · Kay Sievers

Cool, a real bug :)
Thanks, for the patch. I think it would be better to skip lenghth exceeding lines instead of cutting it and continue. While looking at it I restructured the buffer reading logic a bit and fixed another stupid bug.

Thanks for the cleanup.

You may have overlooked the fix for udev_config.c(parsing udev.conf) in your patch. So, I've adapted the fixes you applied to namedev_parse.c to this file also.

Also, while 'eating' the whitespace the 'count' doesn't get decremented. This leads strncpy to copy the number of whitespace minus 1 characters from the next line. Minus 1 because it copies '\n' from the current line.

		while (isspace(bufline[0])) {
			bufline++;
+			count--;
		}
		.
		.
		.
		strncpy(line, bufline, count);

Included patch(against udev-030) contains the above fixes as well as your fixes.

                --Arun

Signed-off-by Arun Bhanu <[email protected]>

klibc_fixups.c +9 -8

diff -Nru a/klibc_fixups.c b/klibc_fixups.c
@@ -41,8 +41,9 @@
 static unsigned long get_id_by_name(const char *uname, const char *dbfile)
 {
 	unsigned long id = -1;
-	char line[255];
+	char line[LINE_SIZE];
 	char *buf;
+	char *bufline;
 	size_t bufsize;
 	size_t cur;
 	size_t count;

@@ -59,19 +60,19 @@
 	}
 
 	/* loop through the whole file */
-
 	cur = 0;
-	while (1) {
+	while (cur < bufsize) {
 		count = buf_get_line(buf, bufsize, cur);
+		bufline = &buf[cur];
+		cur += count+1;
+
+		if (count >= LINE_SIZE)
+			continue;
 
-		strncpy(line, buf + cur, count);
+		strncpy(line, bufline, count);
 		line[count] = '\0';
 		pos = line;
 
-		cur += count+1;
-		if (cur > bufsize)
-			break;
-
 		/* get name */
 		name = strsep(&pos, ":");
 		if (name = NULL)

namedev_parse.c +75 ~~-43~~

271 lines not on this page · raw message

udev.h +2 -0

diff -Nru a/udev.h b/udev.h
@@ -37,6 +37,8 @@
 #define DEVPATH_SIZE			255
 #define SUBSYSTEM_SIZE			30
 
+#define LINE_SIZE			256
+
 /* length of public data */
 #define UDEVICE_LEN (offsetof(struct udevice, bus_id))

udev_config.c +23 ~~-17~~

diff -Nru a/udev_config.c b/udev_config.c
@@ -127,7 +127,8 @@
 
 static int parse_config_file(void)
 {
-	char line[255];
+	char line[LINE_SIZE];
+	char *bufline;
 	char *temp;
 	char *variable;
 	char *value;

@@ -148,32 +149,37 @@
 	/* loop through the whole file */
 	lineno = 0;
 	cur = 0;
-	while (1) {
+	while (cur < bufsize) {
 		count = buf_get_line(buf, bufsize, cur);
-
-		strncpy(line, buf + cur, count);
-		line[count] = '\0';
-		temp = line;
-		lineno++;
-
+		bufline = &buf[cur];
 		cur += count+1;
-		if (cur > bufsize)
-			break;
-
-		dbg_parse("read '%s'", temp);
+		lineno++;
 
-		/* eat the whitespace at the beginning of the line */
-		while (isspace(*temp))
-			++temp;
+		if (count >= LINE_SIZE) {
+			info("line too long, conf line skipped %s, line %d",
+					udev_config_filename, lineno);
+			continue;
+		}
 
 		/* empty line? */
-		if (*temp = 0x00)
+		if (bufline[0] = '\0' || bufline[0] = '\n')
 			continue;
 
+		/* eat the whitespace */
+		while (isspace(bufline[0])) {
+			bufline++;
+			count--;
+		}
+
 		/* see if this is a comment */
-		if (*temp = COMMENT_CHARACTER)
+		if (bufline[0] = COMMENT_CHARACTER)
 			continue;
 
+		strncpy(line, bufline, count);
+		line[count] = '\0';
+		temp = line;
+		dbg_parse("read '%s'", temp);
+
 		retval = parse_get_pair(&temp, &variable, &value);
 		if (retval != 0)
 			info("%s:%d:%Zd: error parsing '%s'",

Kay Sievers

2004-09-05 18:51 UTC #3 lore raw

In reply to Arun Bhanu

Quoted 7 lines · Arun Bhanu

Quoted 4 lines · Kay Sievers
Cool, a real bug :)
Thanks, for the patch. I think it would be better to skip lenghth exceeding lines instead of cutting it and continue. While looking at it I restructured the buffer reading logic a bit and fixed another stupid bug.
Thanks for the cleanup.

Quoted 8 lines

You may have overlooked the fix for udev_config.c(parsing udev.conf) in your patch. So, I've adapted the fixes you applied to namedev_parse.c to this file also.
Also, while 'eating' the whitespace the 'count' doesn't get decremented. This leads strncpy to copy the number of whitespace minus 1 characters from the next line. Minus 1 because it copies '\n' from the current line.

Huh, bad. Yeah, missed the first and broke the second, hope nobody will drive a airplane with this software :)

Looks good, your patch! Many thanks,
Kay

Greg KH

Applied

2004-09-10 20:03 UTC #4 lore raw

In reply to Arun Bhanu

Quoted 27 lines · Arun Bhanu

Hi Kay,
Quoted 4 lines · Kay Sievers
Cool, a real bug :)
Thanks, for the patch. I think it would be better to skip lenghth exceeding lines instead of cutting it and continue. While looking at it I restructured the buffer reading logic a bit and fixed another stupid bug.
Thanks for the cleanup.
You may have overlooked the fix for udev_config.c(parsing udev.conf) in your patch. So, I've adapted the fixes you applied to namedev_parse.c to this file also.
Also, while 'eating' the whitespace the 'count' doesn't get decremented. This leads strncpy to copy the number of whitespace minus 1 characters from the next line. Minus 1 because it copies '\n' from the current line.
		while (isspace(bufline[0])) {
			bufline++;
(quoted diff) +1 -0
+			count--;
		}
		.
		.
		.
		strncpy(line, bufline, count);

Quoted 2 lines

Included patch(against udev-030) contains the above fixes as well as your fixes.

Applied, thanks.

greg k-h

-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170 Project Admins to receive an Apple iPod Mini FREE for your judgement on who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel