Skip to contentSkip to conversation

read() from driverfs files can read more bytes then requested

PATCH In review Andrey Panin 2 participants 2 messages Feb 7, 2002

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 read() from driverfs files can read more bytes then requested

Hi all,

small program below crashes on read() from driverfs file:

int main(void)

{
	int fd, ret;
	char buf[16];

	fd = open("/var/driver/root/pci0/status", 0);
	ret = read(fd, buf, sizeof(buf));
	close(fd);
}

it's because driverfs_read_file() function blindly uses entry->show() return value without sanity check. As a result userspace process requested 16 bytes, but got ~45 and smashed stack as a bonus. You can also get this effect pressing F3 in Midnight Commander on driverfs files.

Attached patch adds check that returned value is less then requested byte count. I know that actual callback function device_read_status() should also be fixed, but I found this bug after midnight and decided to sleep a little :)

Best regards.

Signature · 2 lines
Andrey Panin            | Embedded systems software engineer
[email protected]        | PGP key: wwwkeys.eu.pgp.net
linux/fs/driverfs/inode.c renamed from linux.2.5.3-dj3/fs/driverfs/inode.c +3 -0
diff -urN -X /usr/dontdiff /linux.2.5.3-dj3/fs/driverfs/inode.c /linux/fs/driverfs/inode.c
@@ -255,6 +255,9 @@
 
 		len = entry->show(dev,page,count,*ppos);
 
+		if (len > count)
+			len = count;
+
 		if (len <= 0) {
 			if (len < 0)
 				retval = len;
#1 lore raw
Quoted 19 lines · Andrey Panin

Hi all,

small program below crashes on read() from driverfs file:

int main(void)

{
	int fd, ret;
	char buf[16];

	fd = open("/var/driver/root/pci0/status", 0);
	ret = read(fd, buf, sizeof(buf));
	close(fd);
}

it's because driverfs_read_file() function blindly uses entry->show() return value without sanity check. As a result userspace process requested 16 bytes, but got ~45 and smashed stack as a bonus. You can also get this effect pressing F3 in Midnight Commander on driverfs files.

Quoted 4 lines

Attached patch adds check that returned value is less then requested byte count. I know that actual callback function device_read_status() should also be fixed, but I found this bug after midnight and decided to sleep a little :)

That sanity check was in there, once upon a time. However, in moving the weight from the driver callbacks to the driverfs read_file() and write_file(), it must have got dropped...

Thank you. It's been applied and will be pushed forward.

-pat