netdev - Re: Alignment in BPF verifier

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: <d4df095e-83bb-8bca-5fb5-7815cdc53d7c@fb.com>
Date:   Tue, 23 May 2017 16:59:50 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Edward Cree <ecree@...arflare.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>
CC:     <alexei.starovoitov@...il.com>, <netdev@...r.kernel.org>,
        Josef Bacik <jbacik@...com>
Subject: Re: Alignment in BPF verifier

On 5/23/17 10:43 AM, Edward Cree wrote:
> Another issue: it looks like the min/max_value handling for subtraction is
>  bogus.  In adjust_reg_min_max_vals() we have
>     if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
>         dst_reg->min_value -= min_val;
>     if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
>         dst_reg->max_value -= max_val;
>  where min_val and max_val refer to the src_reg.
> But surely they should be used the other way round; if (say) 2 <= R1 <= 6
>  and 1 <= R2 <= 4, then this will claim 1 <= (R1 - R2) <= 2, whereas really
>  (R1 - R2) could be anything from -2 to 5.
> This also means that the code just above the switch,
>     if (min_val == BPF_REGISTER_MIN_RANGE)
>         dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
>     if (max_val == BPF_REGISTER_MAX_RANGE)
>         dst_reg->max_value = BPF_REGISTER_MAX_RANGE;
>  is wrong, since e.g. subtracting MAX_RANGE needs to blow our min_value,
>  not our max_value.

right. good catch. I have a feeling we discussed similar thing before.
May be some patch felt through the cracks.
That's the reason the fancy verifier analysis is root only.
I'm assuming you're going to send a fix?
Thanks!