Re: [PATCH v17 1/4] Add flags option to get xattr method paired to __vfs_getxattr
From: Paul Moore <paul@paul-moore.com>
Date: 2020-10-21 01:17:34
Also in:
lkml, selinux
On Tue, Oct 20, 2020 at 3:17 PM Mark Salyzyn [off-list ref] 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 <redacted> Reviewed-by: Jan Kara <jack@suse.cz> Acked-by: Jan Kara <jack@suse.cz> Acked-by: Jeff Layton <jlayton@kernel.org> Acked-by: David Sterba <dsterba@suse.com> Acked-by: Darrick J. Wong <redacted> Acked-by: Mike Marshall <hubcap@omnibond.com> To: linux-fsdevel@vger.kernel.org To: linux-unionfs@vger.kernel.org Cc: Stephen Smalley <redacted> Cc: linux-kernel@vger.kernel.org Cc: linux-security-module@vger.kernel.org Cc: kernel-team@android.com
...
quoted hunk ↗ jump to hunk
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