netdev - Re: [oss-drivers] Re: [PATCH bpf] bpf: verifier: make sure callees don't prune with caller differences

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: <87k1kdu2a5.fsf@netronome.com>
Date:   Thu, 13 Dec 2018 10:52:34 +0000
From:   Jiong Wang <jiong.wang@...ronome.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Edward Cree <ecree@...arflare.com>, alexei.starovoitov@...il.com,
        daniel@...earbox.net, oss-drivers@...ronome.com,
        netdev@...r.kernel.org
Subject: Re: [oss-drivers] Re: [PATCH bpf] bpf: verifier: make sure callees don't prune with caller differences

Jakub Kicinski writes:

<snip>

>> Is that just an optimisation that we can do because we know caller's
>> r1-r5 were scratched by the call?

(resent, got a bounce back from netdev mailing list for previous email)

I had been pondering whether it is really necessary to mark caller saved
registers as scratched. I kind of feel the following code inside
check_func_call might cause valid program rejected:

  /* after the call registers r0 - r5 were scratched */
    for (i = 0; i < CALLER_SAVED_REGS; i++) {
        mark_reg_not_init(env, caller->regs, caller_saved[i]);
        check_reg_arg(env, caller_saved[i], DST_OP_NO_MARK);
    }

Because there is inter-procedure register allocation support in LLVM
(-enable-ipra), which could effectively eliminate register save/restore for
one caller-saved register across function call if the compiler can prove
callee or any other childs on the callgraph doesn't use/clobber this
particular caller-saved register. Then the later sequence in caller after
the call site could just safely read the caller-saved without restoring it
from stack etc. But we are marking all caller-saved as NOT_INIT, such read
will be treated as reading from uninitialized value, so the program will be
rejected.

-enable-ipra is disabled at default at the moment.

Regards,
Jiong

> r0 - r5 of the caller will not be pushed to the stack by JITs and
> therefore those are really dead.  It is an optimization.  We could've
> connected them but they are already marked as WRITTEN in
> check_func_call().
>
>> It looks like, without that, the change is "every reg in all frames
>> needs to be parented to the new-state", which is somewhat easier to
>> understand (though I did have to think for quite a long time before
>> it made sense to me why even that was necessary ;-)
>
> Ack, that was my initial patch actually, but Jiong pointed out it's
> unnecessary and I didn't argue :)  r1 - r5 are marked as WRITTEN and
> UNINIT anyway, and r0 gets its parent from the callees frame
> (prepare_func_exit() links callers r0 parentage chain to previous
> states in the callee).