linux-kernel - Re: [PATCH bpf-next v8 5/7] bpf: Implement bpf_local

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60344fad-f761-0fee-a6ef-4880c45c3e52@chromium.org>
Date:   Tue, 18 Aug 2020 17:10:34 +0200
From:   KP Singh <kpsingh@...omium.org>
To:     Martin KaFai Lau <kafai@...com>
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 v8 5/7] bpf: Implement bpf_local_storage for
 inodes



On 8/18/20 3:27 AM, Martin KaFai Lau wrote:
> On Mon, Aug 03, 2020 at 06:46:53PM +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.
>>
>> Signed-off-by: KP Singh <kpsingh@...gle.com>
>> ---
>>  include/linux/bpf_local_storage.h             |  10 +
>>  include/linux/bpf_lsm.h                       |  21 ++
>>  include/linux/bpf_types.h                     |   3 +
>>  include/uapi/linux/bpf.h                      |  38 +++
>>  kernel/bpf/Makefile                           |   1 +

[...]

ata *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 ERR_PTR(-ENOENT);
> ERR_PTR is returned here...
> 
>> +
>> +	inode_storage = rcu_dereference(bsb->storage);
>> +	if (!inode_storage)
>> +		return NULL;
>> +

[...]

>> +		kfree_rcu(local_storage, rcu);
>> +}
>> +
>> +
>> +static void *bpf_fd_inode_storage_lookup_elem(struct bpf_map *map, void *key)
>> +{
>> +	struct bpf_local_storage_data *sdata;
>> +	struct file *f;
>> +	int fd;
>> +
>> +	fd = *(int *)key;
>> +	f = fcheck(fd);
>> +	if (!f)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	get_file(f);
>> +	sdata = inode_storage_lookup(f->f_inode, map, true);
>> +	fput(f);
>> +	return sdata ? sdata->data : NULL;
> sdata can be ERR_PTR here and a few other cases below.
> 
> May be inode_storage_lookup() should just return NULL.

I think returning NULL is a better option. Thanks!

> 
>> +}
>> +
>> +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 = fcheck(fd);
>> +	if (!f)
>> +		return -EINVAL;
>> +
>> +	get_file(f);> get_file() does atomic_long_inc() instead of atomic_long_inc_not_zero().
> I don't see how that helps here.  Am I missing something?

You are right, this should not not be an fcheck followed by a get_file
rather fcheck followed by get_file_rcu:

#define get_file_rcu_many(x, cnt)	\
	atomic_long_add_unless(&(x)->f_count, (cnt), 0)
#define get_file_rcu(x) get_file_rcu_many((x), 1)
#define file_count(x)	atomic_long_read(&(x)->f_count)

But there is an easier way than all of this and this is to use 
fget_raw which also calls get_file_rcu_many 
and ensures a non-zero count before getting a reference.

- KP

> 
>> +	sdata = bpf_local_storage_update(f->f_inode, map, value, map_flags);
>> +	fput(f);
>> +	return PTR_ERR_OR_ZERO(sdata);
>> +}
>> +