On Tue, Mar 26, 2019 at 8:12 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; wrote: > The implementation of kernfs_security_xattr_*() helpers reuses the > kernfs_node_xattr_*() functions, which take the suffix of the xattr name > and extract full xattr name from it using xattr_full_name(). However, > this function relies on the fact that the suffix passed to xattr > handlers from VFS is always constructed from the full name by just > incerementing the pointer. This doesn't necessarily hold for the callers > of kernfs_security_xattr_*(), so their usage will easily lead to > out-of-bounds access. > > Fix this by converting the helpers to take the full xattr name instead > of just the suffix and moving the reconstruction to the xattr handlers. > We now need to check if the prefix is correct in the helpers, but it > saves us the difficulty of reconstructing the full name from just the > plain suffix. > > Reported-by: kernel test robot <rong.a.chen(a)intel.com&gt; > Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization") > Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook") > Signed-off-by: Ondrej Mosnacek <omosnace(a)redhat.com&gt; > --- > > v2: Rebase on current selinux/next. > > fs/kernfs/inode.c | 38 ++++++++++++++++++-------------------- > include/linux/kernfs.h | 8 ++++---- > security/selinux/hooks.c | 6 +++--- > 3 files changed, 25 insertions(+), 27 deletions(-) Thanks for diagnosing this and providing a patch. I haven't seen any objections, but I do have some questions (below). > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c > index 673ef598d97d..1daa8aa9ec96 100644 > --- a/fs/kernfs/inode.c > +++ b/fs/kernfs/inode.c > @@ -288,28 +288,20 @@ int kernfs_iop_permission(struct inode *inode, int mask) > return generic_permission(inode, mask); > } > > -static int kernfs_node_xattr_get(const struct xattr_handler *handler, > - struct kernfs_node *kn, const char *suffix, > +static int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name, > void *value, size_t size) > { > - const char *name = xattr_full_name(handler, suffix); > - struct kernfs_iattrs *attrs; > - > - attrs = kernfs_iattrs_noalloc(kn); > + struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn); > if (!attrs) > return -ENODATA; > > return simple_xattr_get(&attrs->xattrs, name, value, size); > } > > -static int kernfs_node_xattr_set(const struct xattr_handler *handler, > - struct kernfs_node *kn, const char *suffix, > +static int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name, > const void *value, size_t size, int flags) > { > - const char *name = xattr_full_name(handler, suffix); > - struct kernfs_iattrs *attrs; > - > - attrs = kernfs_iattrs(kn); > + struct kernfs_iattrs *attrs = kernfs_iattrs(kn); > if (!attrs) > return -ENOMEM; > ... > -int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix, > +int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name, > void *value, size_t size) > { > - return kernfs_node_xattr_get(&kernfs_security_xattr_handler, > - kn, suffix, value, size); > + if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX))) > + return -EINVAL; > + > + return kernfs_node_xattr_get(kn, name, value, size); > } > > -int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix, > +int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name, > void *value, size_t size, int flags) > { > - return kernfs_node_xattr_set(&kernfs_security_xattr_handler, > - kn, suffix, value, size, flags); > + if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX))) > + return -EINVAL; > + > + return kernfs_node_xattr_set(kn, name, value, size, flags); > } I think it is reasonable to ask if we even need kernfs_security_xattr_{set|get}()? Can we just call the respective kernfs_node_xattr*() functions instead? I can't imagine the WARN_ON_ONCE check being that important.