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]

Message-ID: <5357659E.1070807@redhat.com>
Date:	Wed, 23 Apr 2014 09:02:54 +0200
From:	Daniel Borkmann <dborkman@...hat.com>
To:	Alexei Starovoitov <ast@...mgrid.com>
CC:	David Miller <davem@...emloft.net>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: filter: initialize A and X registers

On 04/23/2014 06:59 AM, Alexei Starovoitov wrote:
> 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?

I think it would be much cleaner to just prevent such filters that
only contain a 'ret A', or 'ret X' w/o a load into X from attaching.
By that, we would need to do slightly more work on the verifier
side (which seemed to allow such cases since ever), but therefore
can spare us the extra work in fast path.

Only 'ret A' doesn't make sense, and similarly a program that never
loads anything into X, but wants to return it. We already did a
similar thing for the scratch memory store where we checked stores
into M[] and loads from M[] into A. I can take a look if you want ...
I have some pending BPF cleanups anyway from before the merge window.

> 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.
> Sorry about this slip.
--
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