Date: Tue, 19 Jan 2016 15:58:12 -0800 From: Kees Cook <keescook@...omium.org> To: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com> Cc: elena.reshetova@...el.com, "Schaufler, Casey" <casey.schaufler@...el.com>, "Leibowitz, Michael" <michael.leibowitz@...el.com> Subject: Re: Understanding the JIT Hardening feature Hi, On Mon, Jan 18, 2016 at 6:34 AM, Daniel Borkmann <daniel@...earbox.net> wrote: > Hi Elena, > > On 01/18/2016 10:22 AM, Reshetova, Elena wrote: > [...] >> >> I got to spend some time reading the 3.15 grsecurity patch with regards to >> JIT hardening feature and wanted to share my thoughts on what the patch >> was >> attempting to do. >> >> It seems that the bulk of changes was done in bpf_jit_compile() function >> (corresponds to the do_jit() 4.4 function). >> >> The way how it was done was to generate a random value (randkey = >> prandom_u32();) and then use that value to dilute (by xor with this value) >> the the four cases of operations: >> >> case BPF_S_ALU_MUL_K: /* A *= K */ >> >> case BPF_S_ALU_MOD_K: /* A %= K; */ >> >> case BPF_S_ALU_DIV_K: /* A /= K */ >> >> case BPF_S_ALU_AND_X: > > > Hm, you mean BPF_S_ALU_AND_K I presume? > > Yeah, that mitigation as implemented makes absolute sense to me. You blind > at > least the 32bit constants away by xor with randkey. > > [...] >> >> Another part of the patch was making changes to the bpf_alloc_binary() >> function. That part I don't really understand since it didn't seem to make >> any security improvements, but merely setting the header length to be 128 >> ad >> changing the bpf_binary_header pointer structure. If this change is to be >> moved to the latest kernel, modifications to the >> <http://lxr.free-electrons.com/ident?i=bpf_jit_binary_alloc> >> bpf_jit_binary_alloc() function (kernel/bpf/core.c) are needed as well as >> changes in <http://lxr.free-electrons.com/ident?i=bpf_binary_header> >> bpf_binary_header pointer structure (include/linux/filter.h). But again, I >> do not understand what security improvements these changes make and why >> they >> were done at the first place. > > > I don't know the patch set deeply enough, but that part, I believe, is a PaX > specific simplification. It doesn't need the struct bpf_binary_header and > can > thus remove this bit of code. The image is written with pax_open_kernel()/ > pax_close_kernel() pair, and f.e. set_memory_ro() can be removed as PaX > already > comes with native protections that handle such things. I would agree: this was likely improvements for KERNEXEC and CONSTIFY. Daniel, did we end up with a generic way to make sure all the JITs end up in read-only memory once they're done being built? I know it got fixed in a few cases, but it wasn't clear to me if they all got fixed. > >> The last change from the patch was done in EMIT_COND_JMP() function (which >> in later kernels is included into emit_cond_jmp switch statement), which >> was >> adding a conditional jz into the flow based on the randkey value. It was >> affecting the following cases: >> >> case BPF_S_ALU_DIV_X: /* A /= X; */ >> case BPF_S_ALU_MOD_X: /* A %= X; */ >> case BPF_S_ANC_IFINDEX: > > > (... for the jumps in 'failure' cases.) > >> as well as conditional branch. > > > Yeah, I think to catch bugs or issues with long jumps, so we effectively BUG > in case jump is off by few bytes. If you look at the kernel git history, > there > was such a case/bug longer time ago that got fixed. > >> The above doesn't seem to go very much in line with what Daniel suggested >> earlier: >> >> "We agreed that the way to go would be to try mitigating it on a BPF >> bytecode level iff feasible. For example, by expanding/rewriting things >> like >> loading constants into a i) load where the constant is xored with a (each >> time newly generated) prandom_u32()/.. value followed by ii) xor on the >> same >> reg with that prandom value itself." >> >> It is also very likely that I didn't understand what you mean Daniel. So >> some clarification questions: >> >> - would you agree with the places where the original grsecurity >> patch attempts to add randomization or do you think places should be >> different? >> >> - for the actual randomization, are you proposing to enhance it >> by >> not only xor to a prandom_u32() but also xor with the same reg? Could you >> explain what do you mean by this part? > > > To answer both, hopefully ... > > I am simply saying that before we go and change every JIT there is in the > kernel, we should give it a try and rewrite BPF instructions first (so BPF > byte code, I mean). That way, JITs would /not/ need to be changed, and would > effectively emit something similar to the image /transparently/. So changes > need to be done in BPF core code instead of JITs. Underlying idea to blind > them out is the same, of course (/how/ you blind them is implementation > detail). > If that path would not be feasible at all for some very good reasons, then > we > can still look into changing the JITs themselves. If you look into the git > history of some of the JITs, there are some rather bad bugs that got fixed. > So, we should try hard to solve this on a generic level first, and if it > turns out to work well **, then we don't need to add further complexity to > each JIT eventually (x86 is just one, but there are arm, arm64, ppc, mips, > sparc, s390 as well). > > **: Also in terms of performance, what I mean by that is: iff due to all > the > mitigation/complexity added, we turn out to become almost as slow as > interpreter > case, then a disabling CONFIG_BPF_JIT in your config might simply be the > better > mitigation option ;). I think we should still be faster, but it certainly > needs to be checked, too. > >> I was also trying to find more info about how JIT code itself works, but >> wasn't able to find anything reasonable, so have to make all my statements >> from just reading the code, which turned out isn't the most easiest thing >> to >> understand for smbd not familiar with topic. So, any pointers to the >> reading material, if exist, are very much appreciated. > > > Here's some rudimentary documentation on the topic in general (yeah, could > need some improvements): > > Documentation/networking/filter.txt Elena, as you learn the JIT code, could you improve this documentation, too? That would be quite valuable. :) -Kees -- Kees Cook Chrome OS & Brillo Security
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.