Date:   Thu, 08 Sep 2022 15:58:53 +0200
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Sitnicki <jakub@...udflare.com>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        "open list:CONTROL GROUP (CGROUP)" <cgroups@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Hou Tao <houtao1@...wei.com>,
        Roberto Sassu <roberto.sassu@...wei.com>,
        stable <stable@...r.kernel.org>, Chenbo Feng <fengc@...gle.com>,
        LSM List <linux-security-module@...r.kernel.org>
Subject: Re: [PATCH 1/7] bpf: Add missing fd modes check for map iterators

On Wed, 2022-09-07 at 09:02 -0700, Alexei Starovoitov wrote:
> 

[...]

> > Well, if you write a security module to prevent writes on a map,
> > and
> > user space is able to do it anyway with an iterator, what is the
> > purpose of the security module then?
> 
> sounds like a broken "security module" and nothing else.

Ok, if a custom security module does not convince you, let me make a
small example with SELinux.

I created a small map iterator that sets every value of a map to 5:

SEC("iter/bpf_map_elem")
int write_bpf_hash_map(struct bpf_iter__bpf_map_elem *ctx)
{
	u32 *key = ctx->key;
	u8 *val = ctx->value;

	if (key == NULL || val == NULL)
		return 0;

	*val = 5;
	return 0;
}

I create and pin a map:

# bpftool map create /sys/fs/bpf/map type array key 4 value 1 entries 1
name test

Initially, the content of the map looks like:

# bpftool map dump pinned /sys/fs/bpf/map 
key: 00 00 00 00  value: 00
Found 1 element

I then created a new SELinux type bpftool_test_t, which has only read
permission on maps:

# sesearch -A -s bpftool_test_t -t unconfined_t -c bpf
allow bpftool_test_t unconfined_t:bpf map_read;

So, what I expect is that this type is not able to write to the map.

Indeed, the current bpftool is not able to do it:

# strace -f -etrace=bpf runcon -t bpftool_test_t bpftool iter pin
writer.o /sys/fs/bpf/iter map pinned /sys/fs/bpf/map
bpf(BPF_OBJ_GET, {pathname="/sys/fs/bpf/map", bpf_fd=0, file_flags=0},
144) = -1 EACCES (Permission denied)
Error: bpf obj get (/sys/fs/bpf): Permission denied

This happens because the current bpftool requests to access the map
with read-write permission, and SELinux denies it:

# cat /var/log/audit/audit.log|audit2allow


#============= bpftool_test_t ==============
allow bpftool_test_t unconfined_t:bpf map_write;


The command failed, and the content of the map is still:

# bpftool map dump pinned /sys/fs/bpf/map 
key: 00 00 00 00  value: 00
Found 1 element


Now, what I will do is to use a slightly modified version of bpftool
which requests read-only access to the map instead:

# strace -f -etrace=bpf runcon -t bpftool_test_t ./bpftool iter pin
writer.o /sys/fs/bpf/iter map pinned /sys/fs/bpf/map
bpf(BPF_OBJ_GET, {pathname="/sys/fs/bpf/map", bpf_fd=0,
file_flags=BPF_F_RDONLY}, 16) = 3
libbpf: elf: skipping unrecognized data section(5) .eh_frame
libbpf: elf: skipping relo section(6) .rel.eh_frame for section(5)
.eh_frame

...

bpf(BPF_LINK_CREATE, {link_create={prog_fd=4, target_fd=0,
attach_type=BPF_TRACE_ITER, flags=0}, ...}, 48) = 5
bpf(BPF_OBJ_PIN, {pathname="/sys/fs/bpf/iter", bpf_fd=5, file_flags=0},
16) = 0

That worked, because SELinux grants read-only permission to
bpftool_test_t. However, the map iterator does not check how the fd was
obtained, and thus allows the iterator to be created.

At this point, we have write access, despite not having the right to do
it:

# cat /sys/fs/bpf/iter
# bpftool map dump pinned /sys/fs/bpf/map 
key: 00 00 00 00  value: 05
Found 1 element

The iterator updated the map value.


The patch I'm proposing checks how the map fd was obtained, and if its
modes are compatible with the operations an attached program is allowed
to do. If the fd does not have the required modes, eBPF denies the
creation of the map iterator.

After patching the kernel, I try to run the modified bpftool again:

# strace -f -etrace=bpf runcon -t bpftool_test_t ./bpftool iter pin
writer.o /sys/fs/bpf/iter map pinned /sys/fs/bpf/map
bpf(BPF_OBJ_GET, {pathname="/sys/fs/bpf/map", bpf_fd=0,
file_flags=BPF_F_RDONLY}, 16) = 3
libbpf: elf: skipping unrecognized data section(5) .eh_frame
libbpf: elf: skipping relo section(6) .rel.eh_frame for section(5)
.eh_frame

...

bpf(BPF_LINK_CREATE, {link_create={prog_fd=4, target_fd=0,
attach_type=BPF_TRACE_ITER, flags=0}, ...}, 48) = -1 EPERM (Operation
not permitted)
libbpf: prog 'write_bpf_hash_map': failed to attach to iterator:
Operation not permitted
Error: attach_iter failed for program write_bpf_hash_map

The map iterator cannot be created and the map is not updated:

# bpftool map dump pinned /sys/fs/bpf/map 
key: 00 00 00 00  value: 00
Found 1 element

Roberto

Powered by blists - more mailing lists