Re: [PATCH v3] usercopy: Check valid lifetime via stack depth

[Kees Cook <keescook@chromium.org>]

On Fri, Feb 25, 2022 at 04:01:57PM -0800, Andrew Morton wrote:
> On Fri, 25 Feb 2022 09:33:45 -0800 Kees Cook <keescook@chromium.org> wrote:
> 
> > Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
> > is not available (i.e. everything except x86 with FRAME_POINTER), check
> > a stack object as being at least "current depth valid", in the sense
> > that any object within the stack region but not between start-of-stack
> > and current_stack_pointer should be considered unavailable (i.e. its
> > lifetime is from a call no longer present on the stack).
> > 
> > Introduce ARCH_HAS_CURRENT_STACK_POINTER to track which architectures
> > have actually implemented the common global register alias.
> > 
> > Additionally report usercopy bounds checking failures with an offset
> > from current_stack_pointer, which may assist with diagnosing failures.
> > 
> > The LKDTM USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests
> > (once slightly adjusted in a separate patch) will pass again with
> > this fixed.
> 
> Again, what does this actually do?

One of the things that CONFIG_HARDENED_USERCOPY checks is whether an
object is overlapping the stack at all. If it is, it performs a number
of inexpensive bounds checks. One of the finer-grained checks is whether
an object cross stack frame within the stack region. Doing this with
CONFIG_FRAME_POINTER was cheap/easy. Doing it with ORC is too heavy, and
was left out (a while ago), leaving the courser whole-stack check.

The LKDTM tests try to exercise the cross-frame cases to validate the
defense. They have been failing every since (which was expected). More
below...

> 
> > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> 
> A link to that report would shed some light.  But actually describing
> the user-visible impact right there in the changelog is preferable.

Yes, good point. The bug[1] involves multiple LKDTM tests and their
failure modes, so it wasn't a very clean pointer. But I will include it.

[1] https://github.com/kernelci/kernelci-project/issues/84

> It sounds like a selftest is newly failing, which makes it a
> userspace-visible regression, perhaps?

No, it's been failing since ORC was introduced, but the regression in
coverage (due to switching from FRAME_POINTER to ORC unwinder) was
minimal. While discussing this with Muhammad, I realized we did,
actually, have something that could be tested that was less than "the
entire stack area" and "each specific frame", and that was current stack
depth, so we gain back a little coverage.

> If so, do we have a Fixes: and is a cc:stable warranted?

I don't think it's warranted; it is technically a new feature.

-Kees

-- 
Kees Cook