Attachment 189351 Details for Bug 280431 – patch to avoid races on proc file reads

[patch] patch to avoid races on proc file reads

linux-kernel-test.patch (text/plain), 3.89 KB, created by Neil Horman on 2007-09-06 23:45:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Neil Horman

Created: 2007-09-06 23:45:59 UTC

Size: 3.89 KB

patch

obsolete

>diff -up linux-2.6.9/include/linux/module.h.orig linux-2.6.9/include/linux/module.h
>--- linux-2.6.9/include/linux/module.h.orig	2007-09-06 10:50:48.000000000 -0400
>+++ linux-2.6.9/include/linux/module.h	2007-09-06 10:51:48.000000000 -0400
>@@ -393,6 +393,7 @@ static inline int try_module_get(struct 
> static inline void module_put(struct module *module)
> {
> 	if (module) {
>+		BUG_ON(module_refcount(module) == 0);
> 		unsigned int cpu = get_cpu();
> 		local_dec(&module->ref[cpu].count);
> 		/* Maybe they're waiting for us to drop reference? */
>diff -up linux-2.6.9/fs/proc/generic.c.orig linux-2.6.9/fs/proc/generic.c
>--- linux-2.6.9/fs/proc/generic.c.orig	2007-09-06 15:03:59.000000000 -0400
>+++ linux-2.6.9/fs/proc/generic.c	2007-09-06 15:07:21.000000000 -0400
>@@ -74,6 +74,9 @@ proc_file_read(struct file *file, char _
> 	if (!(page = (char*) __get_free_page(GFP_KERNEL)))
> 		return -ENOMEM;
> 
>+	if (!try_module_get(dp->owner))
>+		return -EIO;
>+
> 	while ((nbytes > 0) && !eof) {
> 		count = min_t(size_t, PROC_BLOCK_SIZE, nbytes);
> 
>@@ -193,6 +196,7 @@ proc_file_read(struct file *file, char _
> 		buf += n;
> 		retval += n;
> 	}
>+	module_put(dp->owner);
> 	free_page((unsigned long) page);
> 	return retval;
> }
>@@ -203,14 +207,19 @@ proc_file_write(struct file *file, const
> {
> 	struct inode *inode = file->f_dentry->d_inode;
> 	struct proc_dir_entry * dp;
>-	
>+	ssize_t ret;	
> 	dp = PDE(inode);
> 
> 	if (!dp->write_proc)
> 		return -EIO;
> 
>+	if (!try_module_get(dp->owner))
>+		return -EIO;
>+
> 	/* FIXME: does this routine need ppos?  probably... */
>-	return dp->write_proc(file, buffer, count, dp->data);
>+	ret = dp->write_proc(file, buffer, count, dp->data);
>+	module_put(dp->owner);
>+	return ret;
> }
> 
> 
>diff -up linux-2.6.9/fs/proc/inode.c.orig linux-2.6.9/fs/proc/inode.c
>--- linux-2.6.9/fs/proc/inode.c.orig	2007-09-06 15:01:22.000000000 -0400
>+++ linux-2.6.9/fs/proc/inode.c	2007-09-06 15:01:58.000000000 -0400
>@@ -69,8 +69,6 @@ static void proc_delete_inode(struct ino
> 	/* Let go of any associated proc directory entry */
> 	de = PROC_I(inode)->pde;
> 	if (de) {
>-		if (de->owner)
>-			module_put(de->owner);
> 		de_put(de);
> 	}
> 	clear_inode(inode);
>@@ -215,8 +213,6 @@ struct inode *proc_get_inode(struct supe
> 			inode->i_size = de->size;
> 		if (de->nlink)
> 			inode->i_nlink = de->nlink;
>-		if (!try_module_get(de->owner))
>-			goto out_fail;
> 		if (de->proc_iops)
> 			inode->i_op = de->proc_iops;
> 		if (de->proc_fops)
>diff -up linux-2.6.9/net/ipv4/netfilter/ip_tables.c.orig linux-2.6.9/net/ipv4/netfilter/ip_tables.c
>--- linux-2.6.9/net/ipv4/netfilter/ip_tables.c.orig	2007-09-06 11:00:21.000000000 -0400
>+++ linux-2.6.9/net/ipv4/netfilter/ip_tables.c	2007-09-06 15:27:24.000000000 -0400
>@@ -1161,6 +1161,7 @@ do_replace(void __user *user, unsigned i
> 	    (newinfo->number <= oldinfo->initial_entries))
> 		module_put(t->me);
> 
>+
> 	/* Get the old counters. */
> 	get_counters(oldinfo, counters);
> 	/* Decrease module usage counts and free resource */
>@@ -1879,7 +1880,21 @@ static int __init init(void)
> 	int i;
> 
> 	for (i = 0; ipt_proc_entry[i].name; i++) {
>-		proc = proc_net_create(ipt_proc_entry[i].name, 0,
>+		/*
>+		 * Ugh
>+		 * Really, Ugh
>+		 * We have a race here with the setting of proc->owner
>+		 * once we create a proc file, if it gets accessed prior
>+		 * to us setting the module owner its possible for
>+		 * a read to silently _not_ increment the module refcnt
>+		 * but to decrement it on the way back from the read.
>+		 * that leads to a refcnt imbalance, which is badness
>+		 * I'm fixing it by moving the module refcnt work to 
>+		 * proc_file_read and proc_file_write, and using the inode
>+		 * permissions check to avoid reads/writes prior to 
>+		 * the module assignment.
>+		 */
>+		proc = proc_net_create(ipt_proc_entry[i].name, S_ISVTX,
> 				       ipt_proc_entry[i].get_info);
> 		if (!proc) {
> 			while (--i >= 0)
>@@ -1888,6 +1903,7 @@ static int __init init(void)
> 			return -ENOMEM;
> 		}
> 		proc->owner = THIS_MODULE;
>+		proc->mode = S_IRUGO;
> 	}
> 	}
> #endif

Actions: View | Diff

Attachments on bug 280431: 189351 | 190381