netdev - Re: [net-next V4 PATCH 1/5] bpf: introduce new bpf cpu map type BPF_MAP_TYPE

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: <b34454a8-1864-d6bc-644b-c2567ab738d7@gmail.com>
Date:   Thu, 5 Oct 2017 11:01:48 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     netdev@...r.kernel.org, jakub.kicinski@...ronome.com,
        "Michael S. Tsirkin" <mst@...hat.com>, pavel.odintsov@...il.com,
        Jason Wang <jasowang@...hat.com>, mchan@...adcom.com,
        peter.waskiewicz.jr@...el.com,
        Daniel Borkmann <borkmann@...earbox.net>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [net-next V4 PATCH 1/5] bpf: introduce new bpf cpu map type
 BPF_MAP_TYPE_CPUMAP

On 10/04/2017 12:02 PM, Alexei Starovoitov wrote:
> On Wed, Oct 04, 2017 at 02:03:45PM +0200, Jesper Dangaard Brouer wrote:
>> The 'cpumap' is primary used as a backend map for XDP BPF helper
>> call bpf_redirect_map() and XDP_REDIRECT action, like 'devmap'.
>>
>> This patch implement the main part of the map.  It is not connected to
>> the XDP redirect system yet, and no SKB allocation are done yet.
>>
>> The main concern in this patch is to ensure the datapath can run
>> without any locking.  This adds complexity to the setup and tear-down
>> procedure, which assumptions are extra carefully documented in the
>> code comments.
>>
>> V2:
>>  - make sure array isn't larger than NR_CPUS
>>  - make sure CPUs added is a valid possible CPU
>>
>> V3: fix nitpicks from Jakub Kicinski <kubakici@...pl>
>>
>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ...
>> +static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
>> +{
>> +	struct bpf_cpu_map *cmap;
>> +	u64 cost;
>> +	int err;
>> +
>> +	/* check sanity of attributes */
>> +	if (attr->max_entries == 0 || attr->key_size != 4 ||
>> +	    attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	cmap = kzalloc(sizeof(*cmap), GFP_USER);
>> +	if (!cmap)
>> +		return ERR_PTR(-ENOMEM);
> 
> just noticed that there is nothing here nor in DEVMAP/SOCKMAP
> that prevents unpriv user to create them.
> I'm not sure it was intentional for DEVMAP/SOCKMAP.
> For CPUMAP I'd suggest to restrict it to root, since it
> suppose to operate with XDP only which is root anyway.
> Note, lpm and lru maps are cap_sys_admin only already.
> 

For DEVMAP I think the same argument applies. DEVMAP is supposed
to operate with XDP only which is CAP_NET_ADMIN restricted so
we should restrict DEVMAP as well.

In the SOCKMAP case although the map can be created programs
can not be attached. So I'll restrict it to CAP_NET_ADMIN as well
until someone has a clear use case for allowing it. I don't have
a use case for non CAP_NET_ADMIN usage and its easier to relax
restrictions later than add them.

I have a couple fixes for sockmap under test so I'll add these
patches as well. Should have the set ready shortly, in a few days.

Thanks,
John