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

Message-ID: <CAG48ez0V5L610YsJQde_r=BZgK1ghc-AuWTn1gF0oyLNNz4ctQ@mail.gmail.com>
Date:   Tue, 9 Apr 2019 20:46:26 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 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?

> +err_put:
> +       fdput(f);
> +       return err;
> +}