Re: [PATCH] kernel: introduce prctl(PR_LOG_UACCESS)

[Kees Cook]

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@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 22, 2021 at 5:30 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > > On Wed, Sep 22, 2021 at 09:23:10AM -0500, Eric W. Biederman wrote:
> > > > Peter Collingbourne <pcc@xxxxxxxxxx> 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