Re: [patch resend] vfs: move executable checking into ->permission()

[Al Viro <viro@ZenIV.linux.org.uk>]

On Wed, Jul 30, 2008 at 03:02:03PM +0200, Miklos Szeredi wrote:
>  static int coda_ioctl_permission(struct inode *inode, int mask)
>  {
> -        return 0;
> +	return check_execute(dentry->d_inode, mask);
>  }

Er?
	a) mismerge from dentry-based variant
	b) I'd say return mask & MAY_EXEC ? -EACCES : 0 - it's *NOT* going to
be an executable file, TYVM.

>  static int hfs_permission(struct inode *inode, int mask)
>  {
>  	if (S_ISREG(inode->i_mode) && mask & MAY_EXEC)
> -		return 0;
> +		return check_execute(inode, mask);
>  	return generic_permission(inode, mask, NULL);
>  }

WTF is going on in that one?  I realize that you are not changing behaviour,
but...

> +int check_execute(struct inode *inode, int mask)
> +{
> +	if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode) &&
> +	    !(inode->i_mode & S_IXUGO))
> +		return -EACCES;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(check_execute);

Umm...  I'm not sure.  For one thing, I'd take check for MAY_EXEC to callers.
For another, quite a few of those might have enough information to make calling
that helper pointless.

> +++ linux-2.6/fs/proc/proc_sysctl.c	2008-07-30 14:39:31.000000000 +0200
> @@ -311,6 +311,9 @@ static int proc_sys_permission(struct in
>  		error = sysctl_perm(head->root, table, mask);
>  
>  	sysctl_head_finish(head);
> +	if (!error)
> +		error = check_execute(inode, mask);
> +
>  	return error;
>  }

No.  If anything, we want non-directories fail MAY_EXEC here, no matter
what i_mode we might have.  Executable files in /proc/sys/* are NOT going
to be allowed, no matter what...

>  	if (mask & ~mode & (MAY_READ | MAY_WRITE | MAY_EXEC))
> -		error = -EACCES;
> -	return error;
> +		return -EACCES;
> +
> +	return check_execute(inode, mask);

That's wrong.  If mask contains MAY_EXEC and we got to calling check_execute(),
we know that ~mode & MAY_EXEC is 0.  IOW, we know that inode->i_mode >> 6 has
bit 0 set.  IOW, we know that inode->i_mode contain S_IXUSR.  IOW, your
check_execute() here is an obfuscated way to spell 0.