On Fri, Mar 29, 2019 at 2:31 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Tue, Mar 26, 2019 at 8:12 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> >
> > 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.
Indeed, it is now much more natural to just expose all xattrs in those
helpers... I concur that the encapsulation doesn't seem to be worth it
any more. Let me do a simplified respin...
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
