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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080731003332.GJ28946@ZenIV.linux.org.uk>
Date:	Thu, 31 Jul 2008 01:33:32 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch resend] vfs: move executable checking into
	->permission()

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/