[RFC] open_by_handle() vs. EA inodes

[RFC] open_by_handle() vs. EA inodes

[Al Viro]

	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?

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

[Trond Myklebust]

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.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Al Viro]

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

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

[Al Viro]

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?

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

[Darrick J. Wong]

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