On Wed, Apr 15, 2020 at 6:59 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > On Tue, Apr 14, 2020 at 3:50 PM Jann Horn <jannh@xxxxxxxxxx> wrote: > > On Tue, Apr 14, 2020 at 9:46 PM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > On Tue, Apr 14, 2020 at 9:07 AM Jann Horn <jannh@xxxxxxxxxx> wrote: > > > > On Fri, Apr 10, 2020 at 10:48 PM Andrii Nakryiko > > > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > On Fri, Apr 10, 2020 at 1:47 AM Jann Horn <jannh@xxxxxxxxxx> wrote: > > > > > > On Fri, Apr 10, 2020 at 1:33 AM Andrii Nakryiko > > > > > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > On Wed, Apr 8, 2020 at 12:42 PM Jann Horn <jannh@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > Hi! > > > > > > > > > > > > > > > > I saw that BPF allows root to create frozen maps, for which the > > > > > > > > verifier then assumes that they contain constant values. However, map > > > > > > > > freezing is pretty wobbly: [...] > > > > > > > > Is there a reason why the verifier doesn't replace loads from frozen > > > > > > > > maps with the values stored in those maps? That seems like it would be > > > > > > > > not only easier to secure, but additionally more performant. > > > > > > > > > > > > > > Verifier doesn't always know exact offset at which program is going to > > > > > > > read read-only map contents. So something like this works: > > > > > > > > > > > > > > const volatile long arr[256]; > > > > > > > > > > > > > > int x = rand() % 256; > > > > > > > int y = arr[x]; > > > > > > > > > > > > > > In this case verifier doesn't really know the value of y, so it can't > > > > > > > be inlined. Then you can have code in which in one branch register is > > > > > > > loaded with known value, but in another branch same register gets some > > > > > > > value at random offset. Constant tracking is code path-sensitive, > > > > > > > while instructions are shared between different code paths. Unless I'm > > > > > > > missing what you are proposing :) > > > > > > > > > > > > Ah, I missed that possibility. But is that actually something that > > > > > > people do in practice? Or would it be okay for the verifier to just > > > > > > assume an unknown value in these cases? > > > > > > > > > > Verifier will assume unknown value for the branch that has variable > > > > > offset. It can't do the same for another branch (with constant offset) > > > > > because it might not yet have encountered branch with variable offset. > > > > > But either way, you were proposing to rewrite instruction and inline > > > > > read constant, and I don't think it's possible because of this. > > > > > > > > Ah, I see what you mean. That sucks. I guess that means that to fix > > > > this up properly in such edgecases, we'd have to, for each memory > > > > read, keep track of all the values that we want to hardcode for it, > > > > and then generate branches in the unlikely case that the instruction > > > > was reached on paths that expect different values? > > > > > > I guess, though that sounds extreme and extremely unlikely. > > > > It just seems kinda silly to me to have extra memory loads if we know > > that those loads will in most cases load the same fixed value on every > > execution... but oh well. > > > > > I'd say > > > the better way would be to implement immutable BPF maps from the time > > > they are created. E.g., at the time of creating map, you specify extra > > > flag BPF_F_IMMUTABLE and specify pointer to a blob of memory with > > > key/value pairs in it. > > > > It seems a bit hacky to me to add a new special interface for > > populating an immutable map. Wouldn't it make more sense to add a flag > > for "you can't use mmap on this map", or "I plan to freeze this map", > > or something like that, and keep the freezing API? > > "you can't use mmap on this map" is default behavior, unless you > specify BPF_F_MMAPABLE. Ah, right. > "I plan to freeze this map" could be added, > but how that would help existing users that freeze and mmap()? > Disallowing those now would be a breaking change. > Currently, libbpf is using freezing for .rodata variables, but it > doesn't mmap() before freezing. Okay, so it sounds like there are probably no actual users that use both BPF_F_MMAPABLE and freezing, and so we can just forbid that combination? That sounds great. > What we are talking about is malicious > user trying to cause a crash, which given everything is under root is > a bit of a moot point. So I don't know if we actually want to fix > anything here, given that lots of filesystems are already broken for a > while for similar reasons... But it's certainly good to know about > issues like this. Well, it's not just "crash", it's full write access to kernel memory. I think such issues should be fixed for robustness reasons; and if BPF does not fix such issues, that goes against the intent of Matthew Garrett's lockdown LSM, and we'll have to block all of BPF in lockdown's integrity mode if we ever want to get to a point where lockdown actually does anything.
