Re: [RFC] open_by_handle() vs. EA inodes

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Jun 29, 2018 at 09:15:48PM +0100, Al Viro wrote:
> On Fri, Jun 29, 2018 at 08:57:32PM +0100, Al Viro wrote:
> > On Fri, Jun 29, 2018 at 07:38:30PM +0000, Trond Myklebust wrote:
> > > On Fri, 2018-06-29 at 19:19 +0100, Al Viro wrote:
> > > > 	On ea_inode-enabled ext4 open_by_handle() (as well as knfsd,
> > > > etc.)
> > > > can get to EA inodes as long as it knows their inumbers - just pass
> > > > it
> > > > an fhandle with zeroed version bytes and the right inumber in it.
> > > > 
> > > > 	AFAICS, it's Not Nice(tm), especially since you can write to
> > > > those,
> > > > whether they are shared or not.
> > > > 
> > > > 	Should we make ext4_nfs_get_inode() check for EXT4_EA_INODE_FL
> > > > and fail if it's set?
> > > 
> > > handle_to_path() requires CAP_DAC_READ_SEARCH capabilities. Isn't that
> > > sufficiently restrictive for open_by_handle()?
> > > 
> > > Concerning knfsd, people can in theory enable subtree checking to
> > > enforce checking whether or not you are in an exported subtree. In
> > > practice that breaks rename, so people are strongly encouraged to
> > > disable subtree checking, and only to export complete filesystems.
> > 
> > Umm...  Do we ever want those accessed via fhandles, capabilities or
> > no capabilities?  IOW, is there any reason for ext4 ->fh_to_dentry()
> > to give access to such inodes?  Those are implementation internals,
> > same as e.g. journal inode...
> 
> Note, BTW, that journal inode will be rejected by
>         if (ino < EXT4_FIRST_INO(sb) && ino != EXT4_ROOT_INO)
>                 return ERR_PTR(-EFSCORRUPTED);
> in ext4_iget_normal(); EA inodes won't be, since their numbers
> are in the normal range.  And looking at the commit that has
> introduced ext4_iget_normal(), I'd say that EA inodes fit the
> description in
> commit f4bb2981024fc91b23b4d09a8817c415396dbabb
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Sun Oct 5 22:56:00 2014 -0400
> 
>     ext4: add ext4_iget_normal() which is to be used for dir tree lookups
> 
>     If there is a corrupted file system which has directory entries that
>     point at reserved, metadata inodes, prohibit them from being used by
>     treating them the same way we treat Boot Loader inodes --- that is,
>     mark them to be bad inodes.  This prohibits them from being opened,
>     deleted, or modified via chmod, chown, utimes, etc.
>     
>     In particular, this prevents a corrupted file system which has a
>     directory entry which points at the journal inode from being deleted
>     and its blocks released, after which point Much Hilarity Ensues.
> 
> anyway.  IOW, it might be better to have that check done not in
> ext4_nfs_get_inode() but in ext4_iget_normal(), as in
> struct inode *ext4_iget_normal(struct super_block *sb, unsigned long ino)
> {
> 	struct inode *res;
>         if (ino < EXT4_FIRST_INO(sb) && ino != EXT4_ROOT_INO)
>                 return ERR_PTR(-EFSCORRUPTED);
> 	res = ext4_iget(sb, ino);
> 	if (IS_ERR(res) || likely(!(EXT4_I(res)->i_flags & EXT4_EA_INODE_FL)))
> 		return res;
> 	iput(res);
> 	return ERR_PTR(-EFSCORRUPTED);
> }
> 
> Objections?

None here. :)

Based on my reading of how ea inodes work, these special inodes should
never be directly reachable by userspace, ever.  Only the ext4 xattr
code should touch them.

--D