netdev - Re: [PATCH net] net: filter: initialize A and X registers

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]

Date:	Wed, 23 Apr 2014 13:38:33 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	David Miller <davem@...emloft.net>
Cc:	Daniel Borkmann <dborkman@...hat.com>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: filter: initialize A and X registers

On Wed, Apr 23, 2014 at 9:50 AM, David Miller <davem@...emloft.net> wrote:
> From: Alexei Starovoitov <ast@...mgrid.com>
> Date: Tue, 22 Apr 2014 21:59:32 -0700
>
>> On Tue, Apr 22, 2014 at 8:57 PM, David Miller <davem@...emloft.net> wrote:
>>> From: Alexei Starovoitov <ast@...mgrid.com>
>>> Date: Tue, 22 Apr 2014 20:18:57 -0700
>>>
>>>> exisiting BPF verifier allows uninitialized access to registers,
>>>> 'ret A' is considered to be a valid filter.
>>>> So initialize A and X to zero to prevent leaking kernel memory
>>>> In the future BPF verifier will be rejecting such filters
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast@...mgrid.com>
>>>
>>> Has the code always been like this?
>>
>> the previous interpreter had:
>> unsigned int sk_run_filter(const struct sk_buff *skb,
>>                            const struct sock_filter *fentry)
>> {
>>         void *ptr;
>>         u32 A = 0;                      /* Accumulator */
>>         u32 X = 0;                      /* Index Register */
>> ...
>> so it wasn't affected.
>>
>>> Did the eBPF changes introduce this problem either directly or
>>> indirectly?
>>
>> this bug is an oversight on my side.
>> I believe in the net-next it should be fixed by making verifier smarter
>> instead of wasting run time cycles to initialize regs.
>> For now the same fix is needed for both net and net-next.
>> We can remove extra assignments when verifier becomes smarter.
>> ebpf verifier that I posted earlier had checks for uninitialized regs and stack.
>> I missed lack of uninit regs part in classic verifier when ebpf verifier and jit
>> were dropped from the patch set.
>
> I think you will need to add the initializations during validation
> rather than fail because existing BPF filters would be accepted and
> run.  You can't just make them fail unexpectedly after all of these
> years.

In this particular case we can indeed make verifier smarter to the point
that once it detects uninitialized A/X in a filter, it will add explicit init
after conversion, so we don't have to pay the penalty at run time.
It's a very good suggestion.

At the same time I think 'ret A' falls into category of the 20 year old bug.
The filters that using undocumented behavior are destined to be
broken sooner or later. tcpdump doesn't generate such filters.
Valid filters will not be affected by strict checking, but I think users
will be actually happy to see kernel rejecting their buggy filters.
I think we can have a global flag for bpf verifier whether to reject
such filters or accept them by adding explicit inits under
'bpf_permissive_loading' flag.
Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html