On Tue, Oct 20, 2020 at 3:17 PM Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
>
> Add a flag option to get xattr method that could have a bit flag of
> XATTR_NOSECURITY passed to it. XATTR_NOSECURITY is generally then
> set in the __vfs_getxattr path when called by security
> infrastructure.
>
> This handles the case of a union filesystem driver that is being
> requested by the security layer to report back the xattr data.
>
> For the use case where access is to be blocked by the security layer.
>
> The path then could be security(dentry) ->
> __vfs_getxattr(dentry...XATTR_NOSECURITY) ->
> handler->get(dentry...XATTR_NOSECURITY) ->
> __vfs_getxattr(lower_dentry...XATTR_NOSECURITY) ->
> lower_handler->get(lower_dentry...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 the target
> context that was blocked.
>
> Without the get handler flag, the path on a union filesystem would be
> the errant security(dentry) -> __vfs_getxattr(dentry) ->
> handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested ->
> security(lower_dentry, log off) -> lower_handler->get(lower_dentry)
> which would report back through the chain no data, and -EACCES.
>
> For selinux for both cases, this would translate to a correctly
> determined blocked access. In the first case with this change a correct avc
> log would be reported, in the second legacy case an incorrect avc log
> would be reported against an uninitialized u:object_r:unlabeled:s0
> context making the logs cosmetically useless for audit2allow.
>
> This patch series is inert and is the wide-spread addition of the
> flags option for xattr functions, and a replacement of __vfs_getxattr
> with __vfs_getxattr(...XATTR_NOSECURITY).
>
> Signed-off-by: Mark Salyzyn <salyzyn@xxxxxxxxxxx>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
> Acked-by: Jan Kara <jack@xxxxxxx>
> Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Acked-by: David Sterba <dsterba@xxxxxxxx>
> Acked-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> Acked-by: Mike Marshall <hubcap@xxxxxxxxxxxx>
> To: linux-fsdevel@xxxxxxxxxxxxxxx
> To: linux-unionfs@xxxxxxxxxxxxxxx
> Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-security-module@xxxxxxxxxxxxxxx
> Cc: kernel-team@xxxxxxxxxxx
...
> diff --git a/fs/xattr.c b/fs/xattr.c
> index cd7a563e8bcd..d6bf5a7e2420 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -345,7 +345,7 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> return PTR_ERR(handler);
> if (!handler->get)
> return -EOPNOTSUPP;
> - error = handler->get(handler, dentry, inode, name, NULL, 0);
> + error = handler->get(handler, dentry, inode, name, NULL, 0, 0);
> if (error < 0)
> return error;
>
> @@ -356,32 +356,20 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> memset(value, 0, error + 1);
> }
>
> - error = handler->get(handler, dentry, inode, name, value, error);
> + error = handler->get(handler, dentry, inode, name, value, error, 0);
> *xattr_value = value;
> return error;
> }
>
> ssize_t
> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
> - void *value, size_t size)
> + void *value, size_t size, int flags)
> {
> const struct xattr_handler *handler;
> -
> - handler = xattr_resolve_name(inode, &name);
> - if (IS_ERR(handler))
> - return PTR_ERR(handler);
> - if (!handler->get)
> - return -EOPNOTSUPP;
> - return handler->get(handler, dentry, inode, name, value, size);
> -}
> -EXPORT_SYMBOL(__vfs_getxattr);
> -
> -ssize_t
> -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> -{
> - struct inode *inode = dentry->d_inode;
> int error;
>
> + if (flags & XATTR_NOSECURITY)
> + goto nolsm;
> error = xattr_permission(inode, name, MAY_READ);
> if (error)
> return error;
> @@ -403,7 +391,19 @@ vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> return ret;
> }
> nolsm:
> - return __vfs_getxattr(dentry, inode, name, value, size);
> + handler = xattr_resolve_name(inode, &name);
> + if (IS_ERR(handler))
> + return PTR_ERR(handler);
> + if (!handler->get)
> + return -EOPNOTSUPP;
> + return handler->get(handler, dentry, inode, name, value, size, flags);
> +}
> +EXPORT_SYMBOL(__vfs_getxattr);
> +
> +ssize_t
> +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> +{
> + return __vfs_getxattr(dentry, dentry->d_inode, name, value, size, 0);
> }
> EXPORT_SYMBOL_GPL(vfs_getxattr);
[NOTE: added the SELinux list to the CC line]
I'm looking at this patchset in earnest for the first time and I'm a
little uncertain about the need for the new XATTR_NOSECURITY flag;
perhaps you can help me understand it better. Looking over this
patch, and quickly looking at the others in the series, it seems as
though XATTR_NOSECURITY is basically used whenever a filesystem has to
call back into the vfs layer (e.g. overlayfs, ecryptfs, etc). Am I
understanding that correctly? If that assumption is correct, I'm not
certain why the new XATTR_NOSECURITY flag is needed; why couldn't
_vfs_getxattr() be used by all of the callers that need to bypass
DAC/MAC with vfs_getxattr() continuing to perform the DAC/MAC checks?
If for some reason _vfs_getxattr() can't be used, would it make more
sense to create a new stripped/special getxattr function for use by
nested filesystems? Based on the number of revisions to this
patchset, I'm sure it can't be that simple so please educate me :)
--
paul moore
www.paul-moore.com
