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.
--
paul moore
www.paul-moore.com
