Hi Andy,
Thanks for these detailed analysis.
> On Jun 30, 2019, at 8:12 AM, Andy Lutomirski <l...@kernel.org> wrote:
>
> On Fri, Jun 28, 2019 at 12:05 PM Song Liu <songliubrav...@fb.com> wrote:
>>
>> Hi Andy,
>>
>>> On Jun 27, 2019, at 4:40 PM, Andy Lutomirski <l...@kernel.org> wrote:
>>>
>>> On 6/27/19 1:19 PM, Song Liu wrote:
>>>> This patch introduce unprivileged BPF access. The access control is
>>>> achieved via device /dev/bpf. Users with write access to /dev/bpf are able
>>>> to call sys_bpf().
>>>> Two ioctl command are added to /dev/bpf:
>>>> The two commands enable/disable permission to call sys_bpf() for current
>>>> task. This permission is noted by bpf_permitted in task_struct. This
>>>> permission is inherited during clone(CLONE_THREAD).
>>>> Helper function bpf_capable() is added to check whether the task has got
>>>> permission via /dev/bpf.
>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 0e079b2298f8..79dc4d641cf3 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -9134,7 +9134,7 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr
>>>> *attr,
>>>> env->insn_aux_data[i].orig_idx = i;
>>>> env->prog = *prog;
>>>> env->ops = bpf_verifier_ops[env->prog->type];
>>>> - is_priv = capable(CAP_SYS_ADMIN);
>>>> + is_priv = bpf_capable(CAP_SYS_ADMIN);
>>>
>>> Huh? This isn't a hardening measure -- the "is_priv" verifier mode allows
>>> straight-up leaks of private kernel state to user mode.
>>>
>>> (For that matter, the pending lockdown stuff should possibly consider this
>>> a "confidentiality" issue.)
>>>
>>>
>>> I have a bigger issue with this patch, though: it's a really awkward way to
>>> pretend to have capabilities. For bpf, it seems like you could make this be
>>> a *real* capability without too much pain since there's only one syscall
>>> there. Just find a way to pass an fd to /dev/bpf into the syscall. If
>>> this means you need a new bpf_with_cap() syscall that takes an extra
>>> argument, so be it. The old bpf() syscall can just translate to
>>> bpf_with_cap(..., -1).
>>>
>>> For a while, I've considered a scheme I call "implicit rights". There
>>> would be a directory in /dev called /dev/implicit_rights. This would
>>> either be part of devtmpfs or a whole new filesystem -- it would *not* be
>>> any other filesystem. The contents would be files that can't be read or
>>> written and exist only in memory. You create them with a privileged
>>> syscall. Certain actions that are sensitive but not at the level of
>>> CAP_SYS_ADMIN (use of large-attack-surface bpf stuff, creation of user
>>> namespaces, profiling the kernel, etc) could require an "implicit right".
>>> When you do them, if you don't have CAP_SYS_ADMIN, the kernel would do a
>>> path walk for, say, /dev/implicit_rights/bpf and, if the object exists, can
>>> be opened, and actually refers to the "bpf" rights object, then the action
>>> is allowed. Otherwise it's denied.
>>>
>>> This is extensible, and it doesn't require the rather ugly per-task state
>>> of whether it's enabled.
>>>
>>> For things like creation of user namespaces, there's an existing API, and
>>> the default is that it works without privilege. Switching it to an
>>> implicit right has the benefit of not requiring code changes to programs
>>> that already work as non-root.
>>>
>>> But, for BPF in particular, this type of compatibility issue doesn't exist
>>> now. You already can't use most eBPF functionality without privilege. New
>>> bpf-using programs meant to run without privilege are *new*, so they can
>>> use a new improved API. So, rather than adding this obnoxious ioctl, just
>>> make the API explicit, please.
>>>
>>> Also, please cc: linux-abi next time.
>>
>> Thanks for your inputs.
>>
>> I think we need to clarify the use case here. In this case, we are NOT
>> thinking about creating new tools for unprivileged users. Instead, we
>> would like to use existing tools without root.
>
> I read patch 4, and I interpret it very differently. Patches 2-4 are
> creating a new version of libbpf and a new version of bpftool. Given
> this, I see no real justification for adding a new in-kernel per-task
> state instead of just pushing the complexity into libbpf.
I am not sure whether we are on the same page. Let me try an example,
say we have application A, which calls sys_bpf().
Before the series: we have to run A with root;
After the series: we add a special user with access to /dev/bpf, and
run A with this special user.
If we look at the whole system, I would say we are more secure after
the series.
I am not trying to make an extreme example here, because this use case
is the motivation here.
To stay safe, we have to properly manage the permission of /dev/bpf.
This is just like we need to properly manage access to /etc/sudoers and
/dev/mem.
Does this make sense?
Thanks,
Song
