[Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
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.
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_NOSECUIRTY) ->
handler->get(dentry...XATTR_NOSECURITY) ->
__vfs_getxattr(lower_dentry...XATTR_NOSECUIRTY) ->
lower_handler->get(lower_dentry...XATTR_NOSECUIRTY)
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
Cc: Stephen Smalley
Cc: linux-ker...@vger.kernel.org
Cc: kernel-t...@android.com
Cc: linux-security-mod...@vger.kernel.org
Cc: sta...@vger.kernel.org # 4.4, 4.9, 4.14 & 4.19
---
Removed from an overlayfs patch set, and made independent.
Expect this to be the basis of some security improvements.
---
fs/9p/acl.c | 3 ++-
fs/9p/xattr.c | 3 ++-
fs/afs/xattr.c| 6 +++---
fs/btrfs/xattr.c | 3 ++-
fs/ceph/xattr.c | 3 ++-
fs/cifs/xattr.c | 2 +-
fs/ecryptfs/inode.c | 6 --
fs/ecryptfs/mmap.c| 2 +-
fs/ext2/xattr_trusted.c | 2 +-
fs/ext2/xattr_user.c | 2 +-
fs/ext4/xattr_security.c | 2 +-
fs/ext4/xattr_trusted.c | 2 +-
fs/ext4/xattr_user.c | 2 +-
fs/f2fs/xattr.c | 4 ++--
fs/fuse/xattr.c | 4 ++--
fs/gfs2/xattr.c | 3 ++-
fs/hfs/attr.c | 2 +-
fs/hfsplus/xattr.c| 3 ++-
fs/hfsplus/xattr_trusted.c| 3 ++-
fs/hfsplus/xattr_user.c | 3 ++-
fs/jffs2/security.c | 3 ++-
fs/jffs2/xattr_trusted.c | 3 ++-
fs/jffs2/xattr_user.c | 3 ++-
fs/jfs/xattr.c| 5 +++--
fs/kernfs/inode.c | 3 ++-
fs/nfs/nfs4proc.c | 6 --
fs/ocfs2/xattr.c | 9 +---
fs/orangefs/xattr.c | 3 ++-
fs/overlayfs/super.c | 8 ---
fs/posix_acl.c| 2 +-
fs/reiserfs/xattr_security.c | 3 ++-
fs/reiserfs/xattr_trusted.c | 3 ++-
fs/reiserfs/xattr_user.c | 3 ++-
fs/squashfs/xattr.c | 2 +-
fs/xattr.c| 36 +++
fs/xfs/xfs_xattr.c| 3 ++-
include/linux/xattr.h | 9
include/uapi/linux/xattr.h| 5 +++--
mm/shmem.c| 3 ++-
net/socket.c | 3 ++-
security/commoncap.c | 6 --
security/integrity/evm/evm_main.c | 3 ++-
security/selinux/hooks.c | 11 ++
security/smack/smack_lsm.c| 5 +++--
44 files changed, 119 insertions(+), 81 deletions(-)
diff --git a/fs/9p/acl.c b/fs/9p/acl.c
index 6261719f6f2a..cb14e8b312bc 100644
--- a/fs/9p/acl.c
+++ b/fs/9p/acl.c
@@ -214,7 +214,8 @@ int v9fs_acl_mode(struct inode *dir, umode_t *modep,
static int v9fs_xattr_get_acl(const struct xattr_handler *handler,
struct dentry *dentry, struct inode *inode,
- const char *name, void *buffer, size_t size)
+ const char *name, void *buffer, size_t size,
+ int flags)
{
struct v9fs_session_info *v9ses;
struct posix_acl *acl;
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
index ac8ff8ca4c11..5cfa772452fd 100644
--- a/fs/9p/xattr.c
+++ b/fs/9p/xattr.c
@@ -139,7 +139,8 @@ ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer,
size_t buffer_size)
static int v9fs_xattr_handler_get(const struct xattr_handler *handler,
struct dentry *dentry, st
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
Hi Mark,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Mark-Salyzyn/Add-flags-option-to-get-xattr-method-paired-to-__vfs_getxattr/20190813-124612
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
>> fs/ubifs/xattr.c:699:9: error: initialization from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
.get = xattr_get,
^
fs/ubifs/xattr.c:699:9: note: (near initialization for
'ubifs_user_xattr_handler.get')
fs/ubifs/xattr.c:705:9: error: initialization from incompatible pointer type
[-Werror=incompatible-pointer-types]
.get = xattr_get,
^
fs/ubifs/xattr.c:705:9: note: (near initialization for
'ubifs_trusted_xattr_handler.get')
fs/ubifs/xattr.c:712:9: error: initialization from incompatible pointer type
[-Werror=incompatible-pointer-types]
.get = xattr_get,
^
fs/ubifs/xattr.c:712:9: note: (near initialization for
'ubifs_security_xattr_handler.get')
cc1: some warnings being treated as errors
--
>> drivers/staging//erofs/xattr.c:492:9: error: initialization from
>> incompatible pointer type [-Werror=incompatible-pointer-types]
.get = erofs_xattr_generic_get,
^~~
drivers/staging//erofs/xattr.c:492:9: note: (near initialization for
'erofs_xattr_user_handler.get')
drivers/staging//erofs/xattr.c:499:9: error: initialization from
incompatible pointer type [-Werror=incompatible-pointer-types]
.get = erofs_xattr_generic_get,
^~~
drivers/staging//erofs/xattr.c:499:9: note: (near initialization for
'erofs_xattr_trusted_handler.get')
drivers/staging//erofs/xattr.c:506:9: error: initialization from
incompatible pointer type [-Werror=incompatible-pointer-types]
.get = erofs_xattr_generic_get,
^~~
drivers/staging//erofs/xattr.c:506:9: note: (near initialization for
'erofs_xattr_security_handler.get')
cc1: some warnings being treated as errors
--
>> fs//afs/xattr.c:156:12: error: initialization from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
.get= afs_xattr_get_acl,
^
fs//afs/xattr.c:156:12: note: (near initialization for
'afs_xattr_afs_acl_handler.get')
fs//afs/xattr.c:327:9: error: initialization from incompatible pointer type
[-Werror=incompatible-pointer-types]
.get = afs_xattr_get_yfs,
^
fs//afs/xattr.c:327:9: note: (near initialization for
'afs_xattr_yfs_handler.get')
cc1: some warnings being treated as errors
vim +699 fs/ubifs/xattr.c
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 696
dfaf8d2aeca482 Ben Dooks 2016-06-21 697 static const struct
xattr_handler ubifs_user_xattr_handler = {
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 698 .prefix =
XATTR_USER_PREFIX,
ade46c3a6029de Richard Weinberger 2016-09-19 @699 .get = xattr_get,
ade46c3a6029de Richard Weinberger 2016-09-19 700 .set = xattr_set,
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 701 };
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 702
:: The code at line 699 was first introduced by commit
:: ade46c3a6029dea49dbc6c7734b0f6a78d3f104c ubifs: Export xattr get and set
functions
:: TO: Richard Weinberger
:: CC: Richard Weinberger
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -30,10 +30,10 @@ struct xattr_handler {
> const char *prefix;
> int flags; /* fs private flags */
> bool (*list)(struct dentry *dentry);
> - int (*get)(const struct xattr_handler *, struct dentry *dentry,
> + int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> struct inode *inode, const char *name, void *buffer,
> -size_t size);
> - int (*set)(const struct xattr_handler *, struct dentry *dentry,
> +size_t size, int flags);
> + int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> struct inode *inode, const char *name, const void *buffer,
> size_t size, int flags);
Wow, 7 arguments. Isn't there some nice rule of thumb that says once
you get more then 5, a function becomes impossible to understand?
Surely this could be a structure passed in here somehow, that way when
you add the 8th argument in the future, you don't have to change
everything yet again? :)
I don't have anything concrete to offer as a replacement fix for this,
but to me this just feels really wrong...
thanks,
greg k-h
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/13/19 1:48 AM, Greg Kroah-Hartman wrote:
On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -30,10 +30,10 @@ struct xattr_handler {
const char *prefix;
int flags; /* fs private flags */
bool (*list)(struct dentry *dentry);
- int (*get)(const struct xattr_handler *, struct dentry *dentry,
+ int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
struct inode *inode, const char *name, void *buffer,
- size_t size);
- int (*set)(const struct xattr_handler *, struct dentry *dentry,
+ size_t size, int flags);
+ int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
struct inode *inode, const char *name, const void *buffer,
size_t size, int flags);
Wow, 7 arguments. Isn't there some nice rule of thumb that says once
you get more then 5, a function becomes impossible to understand?
This is a method with a pot-pourri of somewhat intuitive useful, but not
always necessary, arguments, the additional argument does not complicate
the function(s) AFAIK, but maybe its usage. Most functions do not even
reference handler, the inode is typically a derivative of dentry, The
arguments most used are the name of the attribute and the buffer/size
the results are to be placed into.
The addition of flags is actually a pattern borrowed from the [.]set
method, which provides at least 32 bits of 'control' (of which we added
only one). Before, it was an anti-pattern.
Surely this could be a structure passed in here somehow, that way when
you add the 8th argument in the future, you don't have to change
everything yet again? :)
Just be happy I provided int flags, instead of bool no_security ;->
there are a few bits there that can be used in the future.
I don't have anything concrete to offer as a replacement fix for this,
but to me this just feels really wrong...
I went through 6 different alternatives (in the overlayfs security fix
patch set) until I found this one that resonated with the security and
filesystem stakeholders. The one was a direct result of trying to reduce
the security attack surface. This code was created by threading a
needle, and evolution. I am game for a 7th alternative to solve the
unionfs set of recursive calls into acquiring the extended attributes.
-- Mark
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:
> On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -30,10 +30,10 @@ struct xattr_handler {
> > const char *prefix;
> > int flags; /* fs private flags */
> > bool (*list)(struct dentry *dentry);
> > - int (*get)(const struct xattr_handler *, struct dentry *dentry,
> > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> >struct inode *inode, const char *name, void *buffer,
> > - size_t size);
> > - int (*set)(const struct xattr_handler *, struct dentry *dentry,
> > + size_t size, int flags);
> > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> >struct inode *inode, const char *name, const void *buffer,
> >size_t size, int flags);
>
> Wow, 7 arguments. Isn't there some nice rule of thumb that says once
> you get more then 5, a function becomes impossible to understand?
>
> Surely this could be a structure passed in here somehow, that way when
> you add the 8th argument in the future, you don't have to change
> everything yet again? :)
>
> I don't have anything concrete to offer as a replacement fix for this,
> but to me this just feels really wrong...
How about something like:
struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
const void *buffer;
size_t size;
int flags;
};
int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);
--
James Morris
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Fri, Aug 16, 2019 at 05:20:36AM +1000, James Morris wrote:
> On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:
>
> > On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
> > > --- a/include/linux/xattr.h
> > > +++ b/include/linux/xattr.h
> > > @@ -30,10 +30,10 @@ struct xattr_handler {
> > > const char *prefix;
> > > int flags; /* fs private flags */
> > > bool (*list)(struct dentry *dentry);
> > > - int (*get)(const struct xattr_handler *, struct dentry *dentry,
> > > + int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
> > > struct inode *inode, const char *name, void *buffer,
> > > -size_t size);
> > > - int (*set)(const struct xattr_handler *, struct dentry *dentry,
> > > +size_t size, int flags);
> > > + int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
> > > struct inode *inode, const char *name, const void *buffer,
> > > size_t size, int flags);
> >
> > Wow, 7 arguments. Isn't there some nice rule of thumb that says once
> > you get more then 5, a function becomes impossible to understand?
> >
> > Surely this could be a structure passed in here somehow, that way when
> > you add the 8th argument in the future, you don't have to change
> > everything yet again? :)
> >
> > I don't have anything concrete to offer as a replacement fix for this,
> > but to me this just feels really wrong...
>
> How about something like:
>
> struct xattr_gs_args {
> struct dentry *dentry;
> struct inode *inode;
As he said in a later message, dentry and inode is redundant, only 1 is
needed (dentry I think?)
> const char *name;
> const void *buffer;
> size_t size;
> int flags;
> };
>
> int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
> int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);
But yes, that would be much much better.
thanks,
greg k-h
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/15/19 12:20 PM, James Morris wrote:
On Tue, 13 Aug 2019, Greg Kroah-Hartman wrote:
On Mon, Aug 12, 2019 at 12:32:49PM -0700, Mark Salyzyn wrote:
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -30,10 +30,10 @@ struct xattr_handler {
const char *prefix;
int flags; /* fs private flags */
bool (*list)(struct dentry *dentry);
- int (*get)(const struct xattr_handler *, struct dentry *dentry,
+ int (*get)(const struct xattr_handler *handler, struct dentry *dentry,
struct inode *inode, const char *name, void *buffer,
- size_t size);
- int (*set)(const struct xattr_handler *, struct dentry *dentry,
+ size_t size, int flags);
+ int (*set)(const struct xattr_handler *handler, struct dentry *dentry,
struct inode *inode, const char *name, const void *buffer,
size_t size, int flags);
Wow, 7 arguments. Isn't there some nice rule of thumb that says once
you get more then 5, a function becomes impossible to understand?
Surely this could be a structure passed in here somehow, that way when
you add the 8th argument in the future, you don't have to change
everything yet again? :)
I don't have anything concrete to offer as a replacement fix for this,
but to me this just feels really wrong...
How about something like:
struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
const void *buffer;
size_t size;
int flags;
};
int (*get)(const struct xattr_handler *handler, struct xattr_gs_args *args);
int (*set)(const struct xattr_handler *handler, struct xattr_gs_args *args);
Good Idea, but using the same argument structure for set and get I would
be concerned about the loss of compiler protection for the buffer
argument; it is void* for get, and const void* for set. And if we pulled
out buffer (and size since it is paired with it) from the structure to
solve, the 'mixed' argument approach (resulting in 4 args) adds to the
difficulty/complexity.
Good news is the same structure(s) can get passed to __vfs_getxattr and
__vfs_setxattr, so one less issue with getting the argument order
correct from the caller.
From an optimization standpoint, passing an argument to a pointer to a
structure assembled on the stack constrains the compiler. Whereas
individual arguments allow for the optimization to place all the
arguments into registers. All modern processors have no issue with tens
of arguments.
So, I will look into what the patch set will look like by splitting into
set and get, and trying to reuse the structure down the call chain.
struct getxattr_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
void *buffer;
size_t size;
int flags;
};
struct setxattr_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
const void *buffer;
size_t size;
int flags;
};
-- Mark
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On Thu, 15 Aug 2019, Mark Salyzyn wrote:
> Good Idea, but using the same argument structure for set and get I would be
> concerned about the loss of compiler protection for the buffer argument;
Agreed, I missed that.
> struct getxattr_args {
> struct dentry *dentry;
> struct inode *inode;
> const char *name;
> void *buffer;
> size_t size;
> int flags;
Does 'get' need flags?
--
James Morris
Re: [Cluster-devel] [PATCH] Add flags option to get xattr method paired to __vfs_getxattr
On 8/15/19 3:27 PM, James Morris wrote:
On Thu, 15 Aug 2019, Mark Salyzyn wrote:
Good Idea, but using the same argument structure for set and get I would be
concerned about the loss of compiler protection for the buffer argument;
Agreed, I missed that.
Sadly, the pattern of
struct getxattr_args args;
memset(&args, 0, sizeof(args));
args. = ...
__vfs_getxattr(&args};
...
__vfs_setxattr(&args);
would be nice, so maybe we need to cool our jets and instead:
struct xattr_gs_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
union {
void *buffer;
const void *value;
};
size_t size;
int flags;
};
value _must_ be referenced for all setxattr operations, buffer for getxattr
operations (how can we enforce that?).
struct getxattr_args {
struct dentry *dentry;
struct inode *inode;
const char *name;
void *buffer;
size_t size;
int flags;
Does 'get' need flags?
:-) That was the _whole_ point of the patch, flags is how we pass in the
recursion so that a security/internal getxattr call has the rights to
acquire the data in the lower layer(s).
-- Mark
