linux-kernel - Re: [patch 04/14] gfs2: dont call 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: <1211533926.3635.552.camel@quoit>
Date:	Fri, 23 May 2008 10:12:06 +0100
From:	Steven Whitehouse <swhiteho@...hat.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	linux-fsdevel@...r.kernel.org, hch@...radead.org,
	viro@...IV.linux.org.uk, linux-kernel@...r.kernel.org
Subject: Re: [patch 04/14] gfs2: dont call permission()

Hi,

On Wed, 2008-05-21 at 19:15 +0200, Miklos Szeredi wrote:
> plain text document attachment (gfs2_permission_fix.patch)
> From: Miklos Szeredi <mszeredi@...e.cz>
> 
> GFS2 calls permission() to verify permissions after locks on the files
> have been taken.
> 
> For this it's sufficient to call gfs2_permission() instead.  This
> results in the following changes:
> 
>   - IS_RDONLY() check is not performed
>   - IS_IMMUTABLE() check is not performed
>   - devcgroup_inode_permission() is not called
>   - security_inode_permission() is not called
> 
> IS_RDONLY() should be unnecessary anyway, as the per-mount read-only
> flag should provide protection against read-only remounts during
> operations.  do_gfs2_set_flags() has been fixed to perform
> mnt_want_write()/mnt_drop_write() to protect against remounting
> read-only.
> 
> IS_IMMUTABLE has beed added to gfs2_do_permission()
> 
That looks ok, but I wonder do we really need gfs2_do_permission() and
gfs2_permission when the only difference seems to be one argument?

> Repeating the security checks seems to be pointless, as they don't
> normally change, and if they do, it's independent of the filesystem
> state.
> 
I hope eventually we can fix this by allowing GFS2 to do its own
lookups, via a suitable VFS library function. I understand that is the
preferred option to replace "open intents" (which we don't currently use
anyway) in the longer term.

> I also suspect the conditional locking in gfs2_do_permission() could
> be cleaned up, due to the removal of the implicit recursion.
> 
In order to be sure we'd have to check that there are no NFS code paths
left which can reach this code. That has usually been the reason for
conditional locking.

In general the patch looks ok to me, and since it doesn't appear to
depend on anything else, I can drop it in my GFS2 git tree if that would
be helpful at this stage,

Steve.


--
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/