netdev - Re: [PATCH v5 bpf-next 2/5] bpf: Introduce sleepable BPF programs

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: <1e9d88c9-c5f2-6def-7afc-aca47a88f4b0@iogearbox.net>
Date:   Wed, 1 Jul 2020 11:17:21 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     davem@...emloft.net, paulmck@...nel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH v5 bpf-next 2/5] bpf: Introduce sleepable BPF programs

On 7/1/20 1:41 AM, Alexei Starovoitov wrote:
> On Wed, Jul 01, 2020 at 01:26:44AM +0200, Daniel Borkmann wrote:
>> On 6/30/20 6:33 AM, Alexei Starovoitov wrote:
>> [...]
>>> +/* list of non-sleepable kernel functions that are otherwise
>>> + * available to attach by bpf_lsm or fmod_ret progs.
>>> + */
>>> +static int check_sleepable_blacklist(unsigned long addr)
>>> +{
>>> +#ifdef CONFIG_BPF_LSM
>>> +	if (addr == (long)bpf_lsm_task_free)
>>> +		return -EINVAL;
>>> +#endif
>>> +#ifdef CONFIG_SECURITY
>>> +	if (addr == (long)security_task_free)
>>> +		return -EINVAL;
>>> +#endif
>>> +	return 0;
>>> +}
>>
>> Would be nice to have some sort of generic function annotation to describe
>> that code cannot sleep inside of it, and then filter based on that. Anyway,
>> is above from manual code inspection?
> 
> yep. all manual. I don't think there is a way to automate it.
> At least I cannot think of one.

Automation might be hard, but maybe semi-automate: we have a cant_migrate()
assertion in __BPF_PROG_RUN() which asserts on cant_sleep() PREEMPT_RT kernels
at least. We originally just has the cant_sleep() there before 37e1d9202225
("bpf: Replace cant_sleep() with cant_migrate()"). So perhaps one way to catch
bugs for sleepable progs is to add a __might_sleep() into __bpf_prog_enter_sleepable()
in order to trigger the assertion generally for DEBUG_ATOMIC_SLEEP configured
kernels when we're in non-sleepable sections? Still not perfect since the code
needs to be exercised first but better than nothing at all.

>> What about others like security_sock_rcv_skb() for example which could be
>> bh_lock_sock()'ed (or, generally hooks running in softirq context)?
> 
> ahh. it's in running in bh at that point? then it should be added to blacklist.

Yep.

Thanks,
Daniel