netdev - Re: [PATCH bpf-next v5 04/16] bpf: add syscall side map freeze support

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] [day] [month] [year] [list]

Date:   Tue, 9 Apr 2019 21:35:06 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jann Horn <jannh@...gle.com>
Cc:     bpf@...r.kernel.org, Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>, joe@...d.net.nz,
        yhs@...com, andrii.nakryiko@...il.com, kafai@...com
Subject: Re: [PATCH bpf-next v5 04/16] bpf: add syscall side map freeze
 support

On 04/09/2019 08:46 PM, Jann Horn wrote:
> On Mon, Apr 8, 2019 at 3:54 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>> This patch adds a new BPF_MAP_FREEZE command which allows to
>> "freeze" the map globally as read-only / immutable from syscall
>> side.
>>
>> Map permission handling has been refactored into map_get_sys_perms()
>> and drops FMODE_CAN_WRITE in case of locked map. Main use case is
>> to allow for setting up .rodata sections from the BPF ELF which
>> are loaded into the kernel, meaning BPF loader first allocates
>> map, sets up map value by copying .rodata section into it and once
>> complete, it calls BPF_MAP_FREEZE on the map fd to prevent further
>> modifications.
>>
>> Right now BPF_MAP_FREEZE only takes map fd as argument while remaining
>> bpf_attr members are required to be zero. I didn't add write-only
>> locking here as counterpart since I don't have a concrete use-case
>> for it on my side, and I think it makes probably more sense to wait
>> once there is actually one. In that case bpf_attr can be extended
>> as usual with a flag field and/or others where flag 0 means that
>> we lock the map read-only hence this doesn't prevent to add further
>> extensions to BPF_MAP_FREEZE upon need.
>>
>> A map creation flag like BPF_F_WRONCE was not considered for couple
>> of reasons: i) in case of a generic implementation, a map can consist
>> of more than just one element, thus there could be multiple map
>> updates needed to set the map into a state where it can then be
>> made immutable, ii) WRONCE indicates exact one-time write before
>> it is then set immutable. A generic implementation would set a bit
>> atomically on map update entry (if unset), indicating that every
>> subsequent update from then onwards will need to bail out there.
>> However, map updates can fail, so upon failure that flag would need
>> to be unset again and the update attempt would need to be repeated
>> for it to be eventually made immutable. While this can be made
>> race-free, this approach feels less clean and in combination with
>> reason i), it's not generic enough. A dedicated BPF_MAP_FREEZE
>> command directly sets the flag, allows all pending operations to
>> finish and caller has the guarantee that map is immutable from
>> syscall side upon successful return, which is also more intuitive
>> from an API point of view. A command name such as BPF_MAP_LOCK has
>> been avoided as it's too close with BPF map spin locks (which already
>> has BPF_F_LOCK flag). BPF_MAP_FREEZE is so far only enabled for
>> privileged users.
> [...]
>> @@ -857,8 +870,7 @@ static int map_update_elem(union bpf_attr *attr)
>>         map = __bpf_map_get(f);
>>         if (IS_ERR(map))
>>                 return PTR_ERR(map);
>> -
>> -       if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
>> +       if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) {
>>                 err = -EPERM;
>>                 goto err_put;
>>         }
> [...]
>> +static int map_freeze(const union bpf_attr *attr)
>> +{
>> +       int err = 0, ufd = attr->map_fd;
>> +       struct bpf_map *map;
>> +       struct fd f;
>> +
>> +       if (CHECK_ATTR(BPF_MAP_FREEZE))
>> +               return -EINVAL;
>> +
>> +       f = fdget(ufd);
>> +       map = __bpf_map_get(f);
>> +       if (IS_ERR(map))
>> +               return PTR_ERR(map);
>> +       if (READ_ONCE(map->frozen)) {
>> +               err = -EBUSY;
>> +               goto err_put;
>> +       }
>> +       if (!capable(CAP_SYS_ADMIN)) {
>> +               err = -EPERM;
>> +               goto err_put;
>> +       }
>> +
>> +       WRITE_ONCE(map->frozen, true);
>> +       synchronize_rcu();
> 
> What is this synchronize_rcu() doing? It seems like your intent might
> be to ensure that all pending writes from the syscall side have
> finished by the time this returns, but functions like
> map_update_elem() aren't in an RCU read-side critical section when
> they check for ->frozen, so that doesn't work, right?

Hmm, yes, true, good point this would only partially work. Considering
that, it's best to just remove the synchronize_rcu() from there. Will
do a v6 with that part removed.

Thanks,
Daniel