Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230515130553.2311248-1-jeffxu@chromium.org> <2bcffc9f-9244-0362-2da9-ece230055320@intel.com>
 <CAEAAPHYdRyZEMp97919errF7SDuYBJoSrD5i1wrTx1sMdr_ZdQ@mail.gmail.com>
 <fbe53dcf-6e21-e4cf-c632-4da8369d7e83@intel.com> <CAEAAPHa=zYyjV5RqvPryRsW7VqY9cJC_-CJW6HKczY0iVsy-bg@mail.gmail.com>
 <d8f2d5c2-6650-c2a6-3a20-25583eee579b@intel.com> <CALmYWFsnGjniVseJKuhKO6eet10Onyk_C0=KNe6ZzXoCiBKZOw@mail.gmail.com>
 <b69f6809-b483-158f-8be9-4976fad918d8@intel.com> <CALmYWFs5Vgosz2JUYWkoc4YwDbiB0tT32MFpo-y6aX4kwuoz8Q@mail.gmail.com>
 <2b14036e-aed8-4212-bc0f-51ec4fe5a5c1@intel.com>
In-Reply-To: <2b14036e-aed8-4212-bc0f-51ec4fe5a5c1@intel.com>
From:   Jeff Xu <jeffxu@google.com>
Date:   Thu, 18 May 2023 13:20:06 -0700
Message-ID: <CALmYWFuSTc5Q7Hrra8FijE11+Y1KiROa=xCZWL1D3ifthrrDMQ@mail.gmail.com>
Subject: Re: [PATCH 0/6] Memory Mapping (VMA) protection using PKU - set 1
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     =?UTF-8?Q?Stephen_R=C3=B6ttger?= <sroettger@google.com>,
        jeffxu@chromium.org, luto@kernel.org, jorgelo@chromium.org,
        keescook@chromium.org, groeck@chromium.org, jannh@google.com,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-mm@kvack.org,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Dave,

Thanks for your email.

On Thu, May 18, 2023 at 8:38=E2=80=AFAM Dave Hansen <dave.hansen@intel.com>=
 wrote:
>
> On 5/17/23 16:48, Jeff Xu wrote:
> > However, there are a few challenges I have not yet worked through.
> > First, the code needs to track when the first signaling entry occurs
> > (saving the PKRU register to the thread struct) and when it is last
> > returned (restoring the PKRU register from the thread struct).
>
> Would tracking signal "depth" work in the face of things like siglongjmp?
>
Thank you for your question! I am eager to learn more about this area
and I worry about blind spots. I will investigate and get back to you.

> Taking a step back...
>
> Here's my concern about this whole thing: it's headed down a rabbit hole
> which is *highly* specialized both in the apps that will use it and the
> attacks it will mitigate.  It probably *requires* turning off a bunch of
> syscalls (like io_uring) that folks kinda like in general.
>
ChromeOS currently disabled io_uring, but it is not required to do so.
io_uring supports the IORING_OP_MADVICE operation, which calls the
do_madvise() function. This means that io_uring will have the same
pkey checks as the madvice() system call.  From that perspective, we
will fully support io_uring for this feature.

> We're balancing that highly specialized mitigation with a feature that
> add new ABI, touches core memory management code and signal handling.
>
The ABI change uses the existing flag field in pkey_alloc() which is
reserved. The implementation is backward compatible with all existing
pkey usages in both kernel and user space.  Or do you have other
concerns about ABI in mind ?

Yes, you are right about the risk of touching core mm code. To
minimize the risk, I try to control the scope of the change (it is
about 3 lines in mprotect, more in munmap but really just 3 effective
lines from syscall entry). I added new self-tests in mm to make sure
it doesn't regress in api behavior. I run those tests before and after
my kernel code change to make sure the behavior remains the same, I
tested it on 5.15 and 6.1 and 6.4-rc1.  Actually, the testing
discovered a behavior change for mprotect() between 6.1 and 6.4  (not
from this patch, there are refactoring works going on in mm) see this
thread [1]
I hope those steps will help to mitigate the risk.

Agreed on signaling handling is a tough part: what do you think about
the approach (modifying PKRU from saved stack after XSAVE), is there a
blocker ?

> On the x86 side, PKRU is a painfully special snowflake.  It's exposed in
> the "XSAVE" ABIs, but not actually managed *with* XSAVE in the kernel.
> This would be making it an even more special snowflake because it would

I admit I'm quite ignorant on XSAVE  to understand the above
statement, and how that is related. Could you explain it to me please
? And what is in your mind that might improve the situation ?

> need new altstack ABI and handling.
>
I thought adding protected memory support to signaling handling is an
independent project with its own weight. As Jann Horn points out in
[2]:  "we could prevent the attacker from corrupting the signal
context if we can protect the signal stack with a pkey."   However,
the kernel will send SIGSEGV when the stack is protected by PKEY,  so
there is a benefit to make this work.  (Maybe Jann can share some more
thoughts on the benefits)

And I believe we could do this in a way with minimum ABI change, as below:
- allocate PKEY with a new flag (PKEY_ALTSTACK)
- at sigaltstack() call, detect the memory is PKEY_ALTSTACK protected,
(similar as what mprotect does in this patch) and save it along with
stack address/size.
- at signaling handling, use the saved info to fill in PKRU.
The ABI change is similar to PKEY_ENFORCE_API, and there is no
backward compatibility issue.

Will these mentioned help our case ? What do you think ?

(Stephan has more info on gains,  as far as I know, V8 engineers have
worked/thought really hard to come to a suitable solution to make
chrome browser safer)

[1] https://lore.kernel.org/linux-mm/20230516165754.pocx4kaagn3yyw3r@revolv=
er/T/
[2] https://docs.google.com/document/d/1OlnJbR5TMoaOAJsf4hHOc-FdTmYK2aDUI7d=
2hfCZSOo/edit?resourcekey=3D0-v9UJXONYsnG5PlCBbcYqIw#

Thanks!
Best regards,
-Jeff