Re: [PATCH v2] Add flags option to get xattr method paired to __vfs_getxattr
On Wed 14-08-19 07:54:16, Mark Salyzyn wrote:
> On 8/14/19 4:00 AM, Jan Kara wrote:
> > On Tue 13-08-19 07:55:06, Mark Salyzyn wrote:
> > ...
> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > index 90dd78f0eb27..71f887518d6f 100644
> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > ...
> > > 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, );
> > > - 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;
> > Hum, is it OK for XATTR_NOSECURITY to skip even the xattr_permission()
> > check? I understand that for reads of security xattrs it actually does not
> > matter in practice but conceptually that seems wrong to me as
> > XATTR_NOSECURITY is supposed to skip just security-module checks to avoid
> > recursion AFAIU.
>
> Good catch I think.
>
> I was attempting to make this change purely inert, no change in
> functionality, only a change in API. Adding a call to xattr_permission would
> incur a change in overall functionality, as it would introduce into the
> current and original __vfs_getxattr a call to xattr_permission that was not
> there before.
>
> (I will have to defer the real answer and requirements to the security
> folks)
>
> AFAIK you are correct, and to make the call would reduce the attack surface,
> trading a very small amount of CPU utilization, for a much larger amount of
> trust.
>
> Given the long history of this patch set (for overlayfs) and the large
> amount of stakeholders, I would _prefer_ to submit a followup independent
> functionality/security change to _vfs_get_xattr _after_ this makes it in.
You're right. The problem was there before. So ack to changing this later.
> > > diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
> > > index c1395b5bd432..1216d777d210 100644
> > > --- a/include/uapi/linux/xattr.h
> > > +++ b/include/uapi/linux/xattr.h
> > > @@ -17,8 +17,9 @@
> > > #if __UAPI_DEF_XATTR
> > > #define __USE_KERNEL_XATTR_DEFS
> > > -#define XATTR_CREATE 0x1 /* set value, fail if attr already
> > > exists */
> > > -#define XATTR_REPLACE0x2 /* set value, fail if attr does not
> > > exist */
> > > +#define XATTR_CREATE 0x1/* set value, fail if attr already
> > > exists */
> > > +#define XATTR_REPLACE 0x2/* set value, fail if attr does not
> > > exist */
> > > +#define XATTR_NOSECURITY 0x4 /* get value, do not involve security
> > > check */
> > > #endif
> > It seems confusing to export XATTR_NOSECURITY definition to userspace when
> > that is kernel-internal flag. I'd just define it in include/linux/xattr.h
> > somewhere from the top of flags space (like 0x4000).
> >
> > Otherwise the patch looks OK to me (cannot really comment on the security
> > module aspect of this whole thing though).
>
> Good point. However, we do need to keep these flags together to reduce
> maintenance risk, I personally abhor two locations for flags bits even if
> one comes from the opposite bit-side; collisions are undetectable at build
> time. Although I have not gone through the entire thought experiment, I am
> expecting that fuse could possibly benefit from this flag (if exposed) since
> it also has a security recursion. That said, fuse is probably the example of
> a gaping wide attack surface if user space had access to it ... your
> xattr_permissions call addition requested above would be realistically, not
> just pedantically, required!
Yeah, flags bits in two places are bad as well. So maybe at least
#ifdef __KERNEL__ bit around the definitiona and a comment that it is
kernel internal flag?
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH v2] 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 next-20190813]
[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/20190814-124805
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 8.1.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=8.1.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
fs/ubifs/xattr.c:326:9: error: conflicting types for 'ubifs_xattr_get'
ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf,
^~~
In file included from fs/ubifs/xattr.c:46:
fs/ubifs/ubifs.h:2006:9: note: previous declaration of 'ubifs_xattr_get' was
here
ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf,
^~~
fs/ubifs/xattr.c: In function 'xattr_get':
fs/ubifs/xattr.c:678:9: error: too few arguments to function
'ubifs_xattr_get'
return ubifs_xattr_get(inode, name, buffer, size);
^~~
fs/ubifs/xattr.c:326:9: note: declared here
ssize_t ubifs_xattr_get(struct inode *host, const char *name, void *buf,
^~~
fs/ubifs/xattr.c: At top level:
>> fs/ubifs/xattr.c:699:9: error: initialization of 'int (*)(const struct
>> xattr_handler *, struct dentry *, struct inode *, const char *, void *,
>> size_t, int)' {aka 'int (*)(const struct xattr_handler *, struct dentry *,
>> struct inode *, const char *, void *, unsigned int, int)'} from
>> incompatible pointer type 'int (*)(const struct xattr_handler *, struct
>> dentry *, struct inode *, const char *, void *, size_t)' {aka 'int (*)(const
>> struct xattr_handler *, struct dentry *, struct inode *, const char *, void
>> *, unsigned int)'} [-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 of 'int (*)(const struct
xattr_handler *, struct dentry *, struct inode *, const char *, void *, size_t,
int)' {aka 'int (*)(const struct xattr_handler *, struct dentry *, struct
inode *, const char *, void *, unsigned int, int)'} from incompatible pointer
type 'int (*)(const struct xattr_handler *, struct dentry *, struct inode *,
const char *, void *, size_t)' {aka 'int (*)(const struct xattr_handler *,
struct dentry *, struct inode *, const char *, void *, unsigned int)'}
[-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 of 'int (*)(const struct
xattr_handler *, struct dentry *, struct inode *, const char *, void *, size_t,
int)' {aka 'int (*)(const struct xattr_handler *, struct dentry *, struct
inode *, const char *, void *, unsigned int, int)'} from incompatible pointer
type 'int (*)(const struct xattr_handler *, struct dentry *, struct inode *,
const char *, void *, size_t)' {aka 'int (*)(const struct xattr_handler *,
struct dentry *, struct inode *, const char *, void *, unsigned int)'}
[-Werror=incompatible-pointer-types]
.get = xattr_get,
^
fs/ubifs/xattr.c:712:9: note: (near initialization for
'ubifs_security_xattr_handler.get')
fs/ubifs/xattr.c: In function 'xattr_get':
fs/ubifs/xattr.c:679:1: warning: control reaches end of non-void function
[-Wreturn-type]
}
^
cc1: some warnings being treated as errors
vim +699 fs/ubifs/xattr.c
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 669
ade46c3a6029de Richard Weinberger 2016-09-19 670 static int xattr_get(const
struct xattr_handler *handler,
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 671
struct dentry *dentry, struct inode *inode,
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 672
const char *name, void *buffer, size_t size)
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 673 {
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 674 dbg_gen("xattr '%s',
ino %lu ('%pd'), buf size %zd", name,
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 675 inode->i_ino,
dentry, size);
2b88fc21cae91e Andreas Gruenbacher 2016-04-22 676
17ce1eb0b64eb2 Richard Weinberger 2016-07-31 677 name =
xattr_full_name(handler, name);
ade46c3a6029de Richard Weinberger 2016-09-19 @678 return
ubifs_xattr_get(inode, name, buffer, size);
2b88fc21cae91e
Re: [PATCH v2] Add flags option to get xattr method paired to __vfs_getxattr
On Tue 13-08-19 07:55:06, Mark Salyzyn wrote:
...
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 90dd78f0eb27..71f887518d6f 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
...
> 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, );
> - 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;
Hum, is it OK for XATTR_NOSECURITY to skip even the xattr_permission()
check? I understand that for reads of security xattrs it actually does not
matter in practice but conceptually that seems wrong to me as
XATTR_NOSECURITY is supposed to skip just security-module checks to avoid
recursion AFAIU.
> diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
> index c1395b5bd432..1216d777d210 100644
> --- a/include/uapi/linux/xattr.h
> +++ b/include/uapi/linux/xattr.h
> @@ -17,8 +17,9 @@
> #if __UAPI_DEF_XATTR
> #define __USE_KERNEL_XATTR_DEFS
>
> -#define XATTR_CREATE 0x1 /* set value, fail if attr already exists */
> -#define XATTR_REPLACE0x2 /* set value, fail if attr does not
> exist */
> +#define XATTR_CREATE 0x1/* set value, fail if attr already exists */
> +#define XATTR_REPLACE 0x2/* set value, fail if attr does not
> exist */
> +#define XATTR_NOSECURITY 0x4 /* get value, do not involve security check */
> #endif
It seems confusing to export XATTR_NOSECURITY definition to userspace when
that is kernel-internal flag. I'd just define it in include/linux/xattr.h
somewhere from the top of flags space (like 0x4000).
Otherwise the patch looks OK to me (cannot really comment on the security
module aspect of this whole thing though).
Honza
--
Jan Kara
SUSE Labs, CR
[PATCH v2] 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 from a
lower layer.
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
---
v2:
Missed a spot: ubifs, erofs and afs.
v1:
Removed from an overlayfs patch set, and made independent.
Expect this to be the basis of some security improvements.
---
drivers/staging/erofs/xattr.c | 3 ++-
fs/9p/acl.c | 3 ++-
fs/9p/xattr.c | 3 ++-
fs/afs/xattr.c| 8 +++
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/ubifs/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 +++--
46 files changed, 123 insertions(+), 84 deletions(-)
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index df40654b9fbb..69440065432c 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -463,7 +463,8 @@ int erofs_getxattr(struct inode *inode, int index,
static int erofs_xattr_generic_get(const struct xattr_handler *handler,
struct dentry *unused, struct inode *inode,
- const char *name, void *buffer, size_t size)
+ const char *name, void *buffer, size_t size,
+ int flags)
{
struct erofs_sb_info *const sbi = EROFS_I_SB(inode);
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
@@
