Re: [PATCH v17 2/4] overlayfs: handle XATTR_NOSECURITY flag for get xattr method

[Mark Salyzyn <salyzyn@android.com>]

On 10/20/20 12:17 PM, Mark Salyzyn wrote:
> Because of the overlayfs getxattr recursion, the incoming inode fails
> to update the selinux sid resulting in avc denials being reported
> against a target context of u:object_r:unlabeled:s0.
>
> Solution is to respond to the XATTR_NOSECURITY flag in get xattr
> method that calls the __vfs_getxattr handler instead so that the
> context can be read in, rather than being denied with an -EACCES
> when vfs_getxattr handler is called.
>
> For the use case where access is to be blocked by the security layer.
>
> The path then would be security(dentry) ->
> __vfs_getxattr({dentry...XATTR_NOSECURITY}) ->
> handler->get({dentry...XATTR_NOSECURITY}) ->
> __vfs_getxattr({realdentry...XATTR_NOSECURITY}) ->
> lower_handler->get({realdentry...XATTR_NOSECURITY}) which
> would report back through the chain data and success as expected,
> the logging security layer at the top would have the data to
> determine the access permissions and report back to the logs and
> the caller that the target context was blocked.
>
> For selinux this would solve the cosmetic issue of the selinux log
> and allow audit2allow to correctly report the rule needed to address
> the access problem.
>
> Check impure, opaque, origin & meta xattr with no sepolicy audit
> (using __vfs_getxattr) since these operations are internal to
> overlayfs operations and do not disclose any data.  This became
> an issue for credential override off since sys_admin would have
> been required by the caller; whereas would have been inherently
> present for the creator since it performed the mount.
>
> This is a change in operations since we do not check in the new
> ovl_do_getxattr function if the credential override is off or not.
> Reasoning is that the sepolicy check is unnecessary overhead,
> especially since the check can be expensive.
>
> Because for override credentials off, this affects _everyone_ that
> underneath performs private xattr calls without the appropriate
> sepolicy permissions and sys_admin capability.  Providing blanket
> support for sys_admin would be bad for all possible callers.
>
> For the override credentials on, this will affect only the mounter,
> should it lack sepolicy permissions. Not considered a security
> problem since mounting by definition has sys_admin capabilities,
> but sepolicy contexts would still need to be crafted.
>
> It should be noted that there is precedence, __vfs_getxattr is used
> in other filesystems for their own internal trusted xattr management.
>
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> To: linux-fsdevel@vger.kernel.org
> To: linux-unionfs@vger.kernel.org
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: linux-doc@vger.kernel.org
>
> v17 - rebase and add inode argument to __Vfs_getxattr
>
> v16 - rebase and merge internal getxattr operations patch
>
> v15 - revert to v13 because xattr_gs_args rejected.
>
> v14 - rebase to use xattr_gs_args.
>
> v13 - rebase to use __vfs_getxattr flags option.
>
> v12 - Added back to patch series as get xattr with flag option.
>
> v11 - Squashed out of patch series and replaced with per-thread flag
>        solution.
>
> v10 - Added to patch series as __get xattr method.
> ---
>   fs/overlayfs/inode.c     | 5 +++--
>   fs/overlayfs/overlayfs.h | 6 ++++--
>   fs/overlayfs/super.c     | 4 ++--
>   3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index b584dca845ba..2b14291beb86 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -378,7 +378,7 @@ int ovl_xattr_set(struct dentry *dentry, struct inode *inode, const char *name,
>   }
>   
>   int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
> -		  void *value, size_t size)
> +		  void *value, size_t size, int flags)
>   {
>   	ssize_t res;
>   	const struct cred *old_cred;
> @@ -386,7 +386,8 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
>   		ovl_i_dentry_upper(inode) ?: ovl_dentry_lower(dentry);
>   
>   	old_cred = ovl_override_creds(dentry->d_sb);
> -	res = vfs_getxattr(realdentry, name, value, size);
> +	res = __vfs_getxattr(realdentry, d_inode(realdentry), name,
> +			     value, size, flags);
>   	revert_creds(old_cred);
>   	return res;
>   }
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index f8880aa2ba0e..d3bf21bd60ab 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -184,7 +184,9 @@ static inline ssize_t ovl_do_getxattr(struct ovl_fs *ofs, struct dentry *dentry,
>   				      size_t size)
>   {
>   	const char *name = ovl_xattr(ofs, ox);
> -	return vfs_getxattr(dentry, name, value, size);
> +	struct ovl_inode *ip = d_inode(dentry);

      struct inode *ip = d_inode(dentry);

<oops> copy and paste error from testing kernel to main