| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200825005249.tu4c54fg36jt3rh4@kafai-mbp.dhcp.thefacebook.com>
Date: Mon, 24 Aug 2020 17:52:49 -0700
From: Martin KaFai Lau <kafai@...com>
To: KP Singh <kpsingh@...omium.org>
CC: <linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
<linux-security-module@...r.kernel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Paul Turner <pjt@...gle.com>, Jann Horn <jannh@...gle.com>,
Florent Revest <revest@...omium.org>
Subject: Re: [PATCH bpf-next v9 5/7] bpf: Implement bpf_local_storage for
inodes
On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@...gle.com>
>
> Similar to bpf_local_storage for sockets, add local storage for inodes.
> The life-cycle of storage is managed with the life-cycle of the inode.
> i.e. the storage is destroyed along with the owning inode.
>
> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> security blob which are now stackable and can co-exist with other LSMs.
>
[ ... ]
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> new file mode 100644
> index 000000000000..b0b283c224c1
> --- /dev/null
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Facebook
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include <linux/rculist.h>
> +#include <linux/list.h>
> +#include <linux/hash.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_local_storage.h>
> +#include <net/sock.h>
> +#include <uapi/linux/sock_diag.h>
> +#include <uapi/linux/btf.h>
> +#include <linux/bpf_lsm.h>
> +#include <linux/btf_ids.h>
> +#include <linux/fdtable.h>
> +
> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
> +
> +static struct bpf_local_storage __rcu **
> +inode_storage_ptr(void *owner)
> +{
> + struct inode *inode = owner;
> + struct bpf_storage_blob *bsb;
> +
> + bsb = bpf_inode(inode);
> + if (!bsb)
> + return NULL;
just noticed this one. NULL could be returned here. When will it happen?
> + return &bsb->storage;
> +}
> +
> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
> + struct bpf_map *map,
> + bool cacheit_lockit)
> +{
> + struct bpf_local_storage *inode_storage;
> + struct bpf_local_storage_map *smap;
> + struct bpf_storage_blob *bsb;
> +
> + bsb = bpf_inode(inode);
> + if (!bsb)
> + return NULL;
lookup is fine since NULL is checked here.
> +
> + inode_storage = rcu_dereference(bsb->storage);
> + if (!inode_storage)
> + return NULL;
> +
> + smap = (struct bpf_local_storage_map *)map;
> + return bpf_local_storage_lookup(inode_storage, smap, cacheit_lockit);
> +}
> +
[ ... ]
> +static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_local_storage_data *sdata;
> + struct file *f;
> + int fd;
> +
> + fd = *(int *)key;
> + f = fget_raw(fd);
> + if (!f)
> + return -EBADF;
> +
> + sdata = bpf_local_storage_update(f->f_inode,
This will be an issue. bpf_local_storage_update() will not check NULL
returned by inode_storage_ptr(). It should be checked here in the inode code
path first before calling the bpf_local_storage_update() since
this case is specific to inode local storage.
Same for the other bpf_local_storage_update() cases.
> + (struct bpf_local_storage_map *)map,
> + value, map_flags);
> + fput(f);
> + return PTR_ERR_OR_ZERO(sdata);
> +}
> +
[ ... ]
> +BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
> + void *, value, u64, flags)
> +{
> + struct bpf_local_storage_data *sdata;
> +
> + if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
> + return (unsigned long)NULL;
> +
> + sdata = inode_storage_lookup(inode, map, true);
> + if (sdata)
> + return (unsigned long)sdata->data;
> +
> + /* This helper must only called from where the inode is gurranteed
> + * to have a refcount and cannot be freed.
> + */
> + if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
> + sdata = bpf_local_storage_update(
> + inode, (struct bpf_local_storage_map *)map, value,
> + BPF_NOEXIST);
> + return IS_ERR(sdata) ? (unsigned long)NULL :
> + (unsigned long)sdata->data;
> + }
> +
> + return (unsigned long)NULL;
> +}
> +
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index 32d32d485451..35f9b19259e5 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -3,6 +3,7 @@
> /*
> * Copyright (C) 2020 Google LLC.
> */
> +#include <linux/bpf_local_storage.h>
Is it needed?
> #include <linux/lsm_hooks.h>
> #include <linux/bpf_lsm.h>
>
> @@ -11,6 +12,7 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> #include <linux/lsm_hook_defs.h>
> #undef LSM_HOOK
> + LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
> };
>
> static int __init bpf_lsm_init(void)
> @@ -20,7 +22,12 @@ static int __init bpf_lsm_init(void)
> return 0;
> }
>
> +struct lsm_blob_sizes bpf_lsm_blob_sizes __lsm_ro_after_init = {
> + .lbs_inode = sizeof(struct bpf_storage_blob),
> +};
> +
> DEFINE_LSM(bpf) = {
> .name = "bpf",
> .init = bpf_lsm_init,
> + .blobs = &bpf_lsm_blob_sizes
> };
Powered by blists - more mailing lists