netdev - Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup

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: <9ded0445-4869-4acc-e67e-0bb6f12a1952@iogearbox.net>
Date:   Tue, 4 Jun 2019 17:30:15 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jonathan Lemon <jonathan.lemon@...il.com>, netdev@...r.kernel.org
Cc:     kernel-team@...com, bjorn.topel@...el.com,
        magnus.karlsson@...el.com, ast@...nel.org
Subject: Re: [PATCH v4 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an
 xskmap

On 06/04/2019 04:54 PM, Daniel Borkmann wrote:
> On 06/03/2019 06:38 PM, Jonathan Lemon wrote:
>> Currently, the AF_XDP code uses a separate map in order to
>> determine if an xsk is bound to a queue.  Instead of doing this,
>> have bpf_map_lookup_elem() return the queue_id, as a way of
>> indicating that there is a valid entry at the map index.
>>
>> Rearrange some xdp_sock members to eliminate structure holes.
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
>> Acked-by: Song Liu <songliubraving@...com>
>> Acked-by: Björn Töpel <bjorn.topel@...el.com>
>> ---
>>  include/net/xdp_sock.h                            |  6 +++---
>>  kernel/bpf/verifier.c                             |  6 +++++-
>>  kernel/bpf/xskmap.c                               |  4 +++-
>>  .../selftests/bpf/verifier/prevent_map_lookup.c   | 15 ---------------
>>  4 files changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
>> index d074b6d60f8a..7d84b1da43d2 100644
>> --- a/include/net/xdp_sock.h
>> +++ b/include/net/xdp_sock.h
>> @@ -57,12 +57,12 @@ struct xdp_sock {
>>  	struct net_device *dev;
>>  	struct xdp_umem *umem;
>>  	struct list_head flush_node;
>> -	u16 queue_id;
>> -	struct xsk_queue *tx ____cacheline_aligned_in_smp;
>> -	struct list_head list;
>> +	u32 queue_id;
>>  	bool zc;
>>  	/* Protects multiple processes in the control path */
>>  	struct mutex mutex;
>> +	struct xsk_queue *tx ____cacheline_aligned_in_smp;
>> +	struct list_head list;
>>  	/* Mutual exclusion of NAPI TX thread and sendmsg error paths
>>  	 * in the SKB destructor callback.
>>  	 */
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 2778417e6e0c..91c730f85e92 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2905,10 +2905,14 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
>>  	 * appear.
>>  	 */
>>  	case BPF_MAP_TYPE_CPUMAP:
>> -	case BPF_MAP_TYPE_XSKMAP:
>>  		if (func_id != BPF_FUNC_redirect_map)
>>  			goto error;
>>  		break;
>> +	case BPF_MAP_TYPE_XSKMAP:
>> +		if (func_id != BPF_FUNC_redirect_map &&
>> +		    func_id != BPF_FUNC_map_lookup_elem)
>> +			goto error;
>> +		break;
>>  	case BPF_MAP_TYPE_ARRAY_OF_MAPS:
>>  	case BPF_MAP_TYPE_HASH_OF_MAPS:
>>  		if (func_id != BPF_FUNC_map_lookup_elem)
>> diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
>> index 686d244e798d..249b22089014 100644
>> --- a/kernel/bpf/xskmap.c
>> +++ b/kernel/bpf/xskmap.c
>> @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map)
>>  
>>  static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
>>  {
>> -	return ERR_PTR(-EOPNOTSUPP);
>> +	struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key);
>> +
>> +	return xs ? &xs->queue_id : NULL;
>>  }
> 
> How do you guarantee that BPf programs don't mess around with the map values
> e.g. overriding xs->queue_id from the lookup? This should be read-only map
> from BPF program side.

(Or via per-cpu scratch var where you move xs->queue_id into and return from here.)