2.6.10 : fs/openpromfs/inode.c : small mistakes makes module oops when writing

2.6.10 : fs/openpromfs/inode.c : small mistakes makes module oops when writing

[Vincent Pelletier]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi.

I'm using kernel 2.6.10 from kernel.org, on an U10 sun ultrasparcIIi.

I'm playing a bit with openprom settings using openpromfs, and I wasn't
able to change the oem-logo. Here is the command and the oops message :

# cat /some/logo/file > /proc/opemprom/options/oem-logo
(same if openpromfs is mounted in /mnt/openprom for example)

Jan  3 12:54:06 usparc kernel: cat(2323): Oops [#1]
Jan  3 12:54:06 usparc kernel: TSTATE: 0000004411009600 TPC: 00000000020520c4 TNPC: 00000000020520c8 Y: 00000000    Tainted: P
Jan  3 12:54:06 usparc kernel: TPC: <property_read+0x4/0xaa0 [openpromfs]>
Jan  3 12:54:06 usparc kernel: g0: 0000000000000000 g1: fffff800076b41e0 g2: 0000000000fffffe g3: 0000000000000000
Jan  3 12:54:06 usparc kernel: g4: fffff800075cd6e0 g5: fffff800076b41e0 g6: fffff80006978000 g7: 0000000070015638
Jan  3 12:54:06 usparc kernel: o0: 00000000000248a0 o1: 0000000000002000 o2: 00000000effff920 o3: 0000000000000000
Jan  3 12:54:06 usparc kernel: o4: 0000000000000c00 o5: 0000000000000c00 sp: fffff8000697b2e1 ret_pc: 0000000000011654
Jan  3 12:54:06 usparc kernel: RPC: <0x11654>
Jan  3 12:54:06 usparc kernel: l0: 0000000000000001 l1: 0000000000000010 l2: 000000002e300047 l3: 0000000000470000
Jan  3 12:54:06 usparc kernel: l4: 000042435f322e30 l5: 0000000000000001 l6: 0000000000000000 l7: 0000000000023800
Jan  3 12:54:06 usparc kernel: i0: fffff800076b41e0 i1: 0000000000000000 i2: 0000000000000000 i3: 0000000000000000
Jan  3 12:54:06 usparc kernel: i4: 0000000000474c49 i5: 0000000000000047 i6: fffff8000697b411 i7: 000000000205340c
Jan  3 12:54:06 usparc kernel: I7: <property_write+0x8ac/0x8e0 [openpromfs]>
Jan  3 12:54:06 usparc kernel: Caller[000000000205340c]: property_write+0x8ac/0x8e0 [openpromfs]
Jan  3 12:54:06 usparc kernel: Caller[000000000048c548]: vfs_write+0xa8/0x100
Jan  3 12:54:06 usparc kernel: Caller[000000000048c62c]: sys_write+0x2c/0x60
Jan  3 12:54:06 usparc kernel: Caller[0000000000411174]: linux_sparc_syscall32+0x34/0x40
Jan  3 12:54:06 usparc kernel: Caller[0000000000012860]: 0x12860
Jan  3 12:54:06 usparc kernel: Instruction DUMP: 01000000  01000000  9de3bed0 <ca5ec000> 03003fff  84102000  821063fe  c65e2010  a8100018

The problem is here in the source file:
Line numbers may not be accurate as I tried some changes.
(line 86)
static ssize_t property_read(struct file *filp, char __user *buf,
			     size_t count, loff_t *ppos)
{
[...]
	if (*ppos >= 0xffffff || count >= 0xffffff)
		return -EINVAL;

(line 321)
static ssize_t property_write(struct file *filp, const char __user *buf,
			      size_t count, loff_t *ppos)
{
[...]
	if (!filp->private_data) {
		i = property_read (filp, NULL, 0, NULL);

So property_read tries to dereference ppos which is set to NULL when
called from property_write. If property_write is called with NULL args
it may oops too, as it uses args the same way.

I think I can write a patch, just tell me if it has chances to be committed in
the kernel (I'm not a kernel dev). If so, where could I find patch
submission guidelines ?

Vincent Pelletier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFB2UPUibN+MXHkAogRAqsSAKCKrF5Q/kdgWZrPF++wEnSupzKw7gCfb10s
dWO0Cf6lahwkS6V8p74n3NM=
=EF1b
-----END PGP SIGNATURE-----

[PATCH] Re: 2.6.10 : fs/openpromfs/inode.c : small mistakes makes module oops when writing

[Vincent Pelletier]

[-- Attachment #1.1: Type: text/plain, Size: 334 bytes --]

Hello.

Here is the patch.

Changelog:
	2005/01/05  Vincent Pelletier  <subdino2004@yahoo.fr>
	inode.c: (nodenum_read, property_read, property_write):
	Protected against NULL parameters. property_read: Returns 0 when
	called with a 0-length buffer. (property_write): Don't expect an
	hex list to begin with a dot.

Vincent Pelletier


[-- Attachment #1.2: openpromfs.diff --]
[-- Type: text/plain, Size: 2888 bytes --]

--- linux-2.6.10/fs/openpromfs/inode.c	2004-12-24 22:34:44.000000000 +0100
+++ linux-2.6.10-openpromfs/fs/openpromfs/inode.c	2005-01-05 16:35:46.345978153 +0100
@@ -67,14 +67,15 @@
 static ssize_t nodenum_read(struct file *file, char __user *buf,
 			    size_t count, loff_t *ppos)
 {
-	struct inode *inode = file->f_dentry->d_inode;
+	struct inode *inode;
 	char buffer[10];
 	
-	if (count < 0 || !inode->u.generic_ip)
+	if (!file || !buf || !ppos || !file->f_dentry || !file->f_dentry->d_inode)
 		return -EINVAL;
-	sprintf (buffer, "%8.8x\n", (u32)(long)(inode->u.generic_ip));
 	if (file->f_pos >= 9)
 		return 0;
+	inode = file->f_dentry->d_inode;
+	sprintf (buffer, "%8.8x\n", (u32)(long)(inode->u.generic_ip));
 	if (count > 9 - file->f_pos)
 		count = 9 - file->f_pos;
 	if (copy_to_user(buf, buffer + file->f_pos, count))
@@ -86,7 +87,7 @@
 static ssize_t property_read(struct file *filp, char __user *buf,
 			     size_t count, loff_t *ppos)
 {
-	struct inode *inode = filp->f_dentry->d_inode;
+	struct inode *inode;
 	int i, j, k;
 	u32 node;
 	char *p, *s;
@@ -94,9 +95,12 @@
 	openprom_property *op;
 	char buffer[64];
 	
-	if (*ppos >= 0xffffff || count >= 0xffffff)
+	if (!filp || count >= 0xffffff || (count && !buf) || (ppos && *ppos >= 0xffffff) || !filp->f_dentry || !filp->f_dentry->d_inode)
 		return -EINVAL;
+	inode = filp->f_dentry->d_inode;
 	if (!filp->private_data) {
+		if(!nodes)
+			return -EIO;
 		node = nodes[(u16)((long)inode->u.generic_ip)].node;
 		i = ((u32)(long)inode->u.generic_ip) >> 16;
 		if ((u16)((long)inode->u.generic_ip) == aliases) {
@@ -166,7 +170,7 @@
 		}
 	} else
 		op = (openprom_property *)filp->private_data;
-	if (!count || !(op->len || (op->flag & OPP_ASCIIZ)))
+	if (!ppos || !count || !(op->len || (op->flag & OPP_ASCIIZ)))
 		return 0;
 	if (op->flag & OPP_STRINGLIST) {
 		for (k = 0, p = op->value; p < op->value + op->len; p++)
@@ -327,7 +331,7 @@
 	void *b;
 	openprom_property *op;
 	
-	if (*ppos >= 0xffffff || count >= 0xffffff)
+	if (!filp || !ppos || *ppos >= 0xffffff || count >= 0xffffff || (count && !buf))
 		return -EINVAL;
 	if (!filp->private_data) {
 		i = property_read (filp, NULL, 0, NULL);
@@ -343,13 +347,11 @@
 		char tmp [9];
 		int forcelen = 0;
 		
-		j = k % 9;
-		for (i = 0; i < count; i++, j++) {
-			if (j == 9) j = 0;
-			if (!j) {
-				char ctmp;
-				if (get_user(ctmp, &buf[i]))
-					return -EFAULT;
+		for (i = 0,j = k; i < count; i++,j++) {
+			char ctmp;
+			if (get_user(ctmp, &buf[i]))
+				return -EFAULT;
+			if (j && !((j+1)%9)) { /* every 9 char exept first of the file */
 				if (ctmp != '.') {
 					if (ctmp != '\n') {
 						if (op->flag & OPP_BINARY)
@@ -363,9 +365,6 @@
 					}
 				}
 			} else {
-				char ctmp;
-				if (get_user(ctmp, &buf[i]))
-					return -EFAULT;
 				if (ctmp < '0' || 
 				    (ctmp > '9' && ctmp < 'A') ||
 				    (ctmp > 'F' && ctmp < 'a') ||

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]

Re: [PATCH] Re: 2.6.10 : fs/openpromfs/inode.c : small mistakes makes module oops when writing

[Al Viro]

On Wed, Jan 05, 2005 at 04:39:21PM +0100, Vincent Pelletier wrote:
> Hello.
> 
> Here is the patch.
> 
> Changelog:
> 	2005/01/05  Vincent Pelletier  <subdino2004@yahoo.fr>
> 	inode.c: (nodenum_read, property_read, property_write):
> 	Protected against NULL parameters. property_read: Returns 0 when
> 	called with a 0-length buffer. (property_write): Don't expect an
> 	hex list to begin with a dot.

NAK.  Patch
	a) loses needed checks
	b) adds utterly bogus ones
	c) is much bigger than it should be

All it really takes is

diff -urN RC10-bk6-base/fs/openpromfs/inode.c RC10-bk6-current/fs/openpromfs/inode.c
--- RC10-bk6-base/fs/openpromfs/inode.c	2004-10-18 17:54:07.000000000 -0400
+++ RC10-bk6-current/fs/openpromfs/inode.c	2005-01-05 12:22:08.710924933 -0500
@@ -94,8 +94,6 @@
 	openprom_property *op;
 	char buffer[64];
 	
-	if (*ppos >= 0xffffff || count >= 0xffffff)
-		return -EINVAL;
 	if (!filp->private_data) {
 		node = nodes[(u16)((long)inode->u.generic_ip)].node;
 		i = ((u32)(long)inode->u.generic_ip) >> 16;
@@ -168,6 +166,8 @@
 		op = (openprom_property *)filp->private_data;
 	if (!count || !(op->len || (op->flag & OPP_ASCIIZ)))
 		return 0;
+	if (*ppos >= 0xffffff || count >= 0xffffff)
+		return -EINVAL;
 	if (op->flag & OPP_STRINGLIST) {
 		for (k = 0, p = op->value; p < op->value + op->len; p++)
 			if (!*p)