Messages in this thread | | | Date | Sat, 25 Sep 2021 19:20:22 -0700 | From | Kees Cook <> | Subject | Re: [PATCH] kernel: introduce prctl(PR_LOG_UACCESS) |
| |
On Fri, Sep 24, 2021 at 02:50:04PM -0700, Peter Collingbourne wrote: > On Wed, Sep 22, 2021 at 8:59 AM Jann Horn <jannh@google.com> wrote: > > > > On Wed, Sep 22, 2021 at 5:30 PM Kees Cook <keescook@chromium.org> wrote: > > > On Wed, Sep 22, 2021 at 09:23:10AM -0500, Eric W. Biederman wrote: > > > > Peter Collingbourne <pcc@google.com> writes: > > > > > This patch introduces a kernel feature known as uaccess logging. > > > > > [...] > > > > [...] > > > > How is logging the kernel's activity like this not a significant > > > > information leak? How is this safe for unprivileged users? > > > [...] > > > Regardless, this is a pretty useful tool for this kind of fuzzing. > > > Perhaps the timing exposure could be mitigated by having the kernel > > > collect the record in a separate kernel-allocated buffer and flush the > > > results to userspace at syscall exit? (This would solve the > > > copy_to_user() recursion issue too.) > > Seems reasonable. I suppose that in terms of timing information we're > already (unavoidably) exposing how long the syscall took overall, and > we probably shouldn't deliberately expose more than that.
Right -- I can't think of anything that can really use this today, but it very much feels like the kind of information that could aid in a timing race.
> That being said, I'm wondering if that has security implications on > its own if it's then possible for userspace to manipulate the kernel > into allocating a large buffer (either at prctl() time or as a result > of getting the kernel to do a large number of uaccesses). Perhaps it > can be mitigated by limiting the size of the uaccess buffer provided > at prctl() time.
There are a lot of exact-size allocation controls already (which I think is an unavoidable but separate issue[1]), but perhaps this could be mitigated by making the reserved buffer be PAGE_SIZE granular?
> > One aspect that might benefit from some clarification on intended > > behavior is: what should happen if there are BPF tracing programs > > running (possibly as part of some kind of system-wide profiling or > > such) that poke around in userspace memory with BPF's uaccess helpers > > (especially "bpf_copy_from_user")? > > I think we should probably be ignoring those accesses, since we cannot > know a priori whether the accesses are directly associated with the > syscall or not, and this is after all a best-effort mechanism.
Perhaps the "don't log this uaccess" flag I suggested could be repurposed by BPF too, as a general "make this access invisible to PR_LOG_UACCESS" flag? i.e. this bit:
> > > Instead of reimplementing copy_*_user() with a new wrapper that > > > bypasses some checks and adds others and has to stay in sync, etc, > > > how about just adding a "recursion" flag? Something like: > > > > > > copy_from_user(...) > > > instrument_copy_from_user(...) > > > uaccess_buffer_log_read(...) > > > if (current->uaccess_buffer.writing) > > > return; > > > uaccess_buffer_log(...) > > > current->uaccess_buffer.writing = true; > > > copy_to_user(...) > > > current->uaccess_buffer.writing = false;
> > > This would likely only make sense for SECCOMP_RET_TRACE or _TRAP if the > > > program wants to collect the results after every syscall. And maybe this > > > won't make any sense across exec (losing the mm that was used during > > > SECCOMP_SET_UACCESS_TRACE_BUFFER). Hmmm. > > > > And then I guess your plan would be that userspace would be expected > > to use the userspace instruction pointer > > (seccomp_data::instruction_pointer) to indicate instructions that > > should be traced?
That could be one way -- but seccomp filters would allow a bunch of ways.
> > > > Or instead of seccomp, you could do it kinda like > > https://www.kernel.org/doc/html/latest/admin-guide/syscall-user-dispatch.html > > , with a prctl that specifies a specific instruction pointer? > > Given a choice between these two options, I would prefer the prctl() > because userspace programs may already be using seccomp filters and > sanitizers shouldn't interfere with it.
That's fair -- the "I wish we could make complex decisions about which syscalls to act on" sounds like seccomp.
> However, in either the seccomp filter or prctl() case, you still have > the problem of deciding where to log to. Keep in mind that you would > need to prevent intervening async signals (that occur between when the > syscall happens and when we read the log) from triggering additional
Could the sig handler also set the "make the uaccess invisible" flag? (It would need to be a "depth" flag, most likely.)
-Kees
[1] https://github.com/KSPP/linux/issues/9
-- Kees Cook
| |