/proc/bus/pci IOCTL breakage

/proc/bus/pci IOCTL breakage

[David Miller]

This change:

commit 786d7e1612f0b0adb6046f19b906609e4fe8b1ba
Author: Alexey Dobriyan <adobriyan@sw.ru>
Date:   Sun Jul 15 23:39:00 2007 -0700

    Fix rmmod/read/write races in /proc entries

Broke ioctl() on /proc/bus/pci/* files for COMPAT platforms.

proc_fops->ioctl() is defined for these PCI device files, and the
COMPAT ioctl is handled via fs/compat_ioctl.c's entries, which makes
it just call the ->ioctl() handler directly.

proc_fops->compat_ioctl is NULL for these files, it isn't needed.

This used to work because we used to jump right to the de->proc_fops,
but now we have these wrappers and proc_reg_compat_ioctl is what
gets called and since proc_fops->compat_ioctl is NULL we return
ENOTTY instead of calling proc_fops->ioctl().

Two ways to fix:

1) Make the PROC wrapper call ->unlocked_ioctl() or ->ioctl()
   as a fallback of ->compat_ioctl is NULL.

2) Make proc_bus_pci_operations provide a .compat_ioctl method,
   but then we'll need to audit the entire tree for cases like
   this and make the same fix.

Because it's easier to validate that all cases are covered,
I think #1 is the preferred fix.

Re: /proc/bus/pci IOCTL breakage

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 26 Jul 2007 19:07:51 -0700 (PDT)

> Two ways to fix:
> 
> 1) Make the PROC wrapper call ->unlocked_ioctl() or ->ioctl()
>    as a fallback of ->compat_ioctl is NULL.
> 
> 2) Make proc_bus_pci_operations provide a .compat_ioctl method,
>    but then we'll need to audit the entire tree for cases like
>    this and make the same fix.
> 
> Because it's easier to validate that all cases are covered,
> I think #1 is the preferred fix.

Here is my suggested fix.

It is important to only provide the compat_ioctl method
if the downstream de->proc_fops does too, otherwise this
utterly confuses the logic in fs/compat_ioctl.c and we
end up doing the right thing.

To be honest, the other unlocked_ioctl fallback scheme in
this file should be scrutinized for similar problems.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 94e2c1a..a5b0dfd 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -386,6 +386,19 @@ static const struct file_operations proc_reg_file_ops = {
 	.release	= proc_reg_release,
 };
 
+#ifdef CONFIG_COMPAT
+static const struct file_operations proc_reg_file_ops_no_compat = {
+	.llseek		= proc_reg_llseek,
+	.read		= proc_reg_read,
+	.write		= proc_reg_write,
+	.poll		= proc_reg_poll,
+	.unlocked_ioctl	= proc_reg_unlocked_ioctl,
+	.mmap		= proc_reg_mmap,
+	.open		= proc_reg_open,
+	.release	= proc_reg_release,
+};
+#endif
+
 struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
 				struct proc_dir_entry *de)
 {
@@ -413,8 +426,15 @@ struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
 		if (de->proc_iops)
 			inode->i_op = de->proc_iops;
 		if (de->proc_fops) {
-			if (S_ISREG(inode->i_mode))
-				inode->i_fop = &proc_reg_file_ops;
+			if (S_ISREG(inode->i_mode)) {
+#ifdef CONFIG_COMPAT
+				if (!de->proc_fops->compat_ioctl)
+					inode->i_fop =
+						&proc_reg_file_ops_no_compat;
+				else
+#endif
+					inode->i_fop = &proc_reg_file_ops;
+			}
 			else
 				inode->i_fop = de->proc_fops;
 		}

Re: /proc/bus/pci IOCTL breakage

[Alexey Dobriyan]

On Thu, Jul 26, 2007 at 11:04:35PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 26 Jul 2007 19:07:51 -0700 (PDT)
> 
> > Two ways to fix:
> > 
> > 1) Make the PROC wrapper call ->unlocked_ioctl() or ->ioctl()
> >    as a fallback of ->compat_ioctl is NULL.
> > 
> > 2) Make proc_bus_pci_operations provide a .compat_ioctl method,
> >    but then we'll need to audit the entire tree for cases like
> >    this and make the same fix.
> > 
> > Because it's easier to validate that all cases are covered,
> > I think #1 is the preferred fix.
>
> Here is my suggested fix.
> 
> It is important to only provide the compat_ioctl method
> if the downstream de->proc_fops does too, otherwise this
> utterly confuses the logic in fs/compat_ioctl.c and we
> end up doing the right thing.

Indeed, my patch broke cases where .compat_ioctl was not supplied and
ioctl was done with compat_sys_ioctl(). 

> To be honest, the other unlocked_ioctl fallback scheme in
> this file should be scrutinized for similar problems.

I checked on test module all (3 + 3 + 1) x 2 combinations of available
methods and ioctl/compat_ioctl accesses. Regression were only in
compat_sys_ioctl(2) part. And your patch fixes all of them.

> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -386,6 +386,19 @@ static const struct file_operations proc_reg_file_ops = {
>  	.release	= proc_reg_release,
>  };
>  
> +#ifdef CONFIG_COMPAT
> +static const struct file_operations proc_reg_file_ops_no_compat = {
> +	.llseek		= proc_reg_llseek,
> +	.read		= proc_reg_read,
> +	.write		= proc_reg_write,
> +	.poll		= proc_reg_poll,
> +	.unlocked_ioctl	= proc_reg_unlocked_ioctl,
> +	.mmap		= proc_reg_mmap,
> +	.open		= proc_reg_open,
> +	.release	= proc_reg_release,
> +};
> +#endif
> +
>  struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
>  				struct proc_dir_entry *de)
>  {
> @@ -413,8 +426,15 @@ struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
>  		if (de->proc_iops)
>  			inode->i_op = de->proc_iops;
>  		if (de->proc_fops) {
> -			if (S_ISREG(inode->i_mode))
> -				inode->i_fop = &proc_reg_file_ops;
> +			if (S_ISREG(inode->i_mode)) {
> +#ifdef CONFIG_COMPAT
> +				if (!de->proc_fops->compat_ioctl)
> +					inode->i_fop =
> +						&proc_reg_file_ops_no_compat;
> +				else
> +#endif
> +					inode->i_fop = &proc_reg_file_ops;
> +			}
>  			else
>  				inode->i_fop = de->proc_fops;
>  		}

Looks good, thanks David.