Message-ID: <db7be4b7c31828d559ad4b1f4a93b76e@codeaurora.org>
Date:   Mon, 22 Feb 2021 12:44:29 +0530
From:   pnagar@...eaurora.org
To:     Ard Biesheuvel <ardb@...nel.org>, Will Deacon <will@...nel.org>
Cc:     Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>, paul@...l-moore.com,
        stephen.smalley.work@...il.com, eparis@...isplace.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        linux-arch <linux-arch@...r.kernel.org>, casey@...aufler-ca.com,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        David Howells <dhowells@...hat.com>, ojeda@...nel.org,
        Prasad Sodagudi <psodagud@...eaurora.org>,
        nmardana@...eaurora.org, johan@...nel.org,
        Joe Perches <joe@...ches.com>, Jessica Yu <jeyu@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] RTIC: selinux: ARM64: Move selinux_state to a separate
 page

On 2021-02-17 15:23, Ard Biesheuvel wrote:
> On Wed, 17 Feb 2021 at 10:42, Will Deacon <will@...nel.org> wrote:
>> 
>> [Please include arm64 and kvm folks for threads involving the stage-2 
>> MMU]
>> 
>> On Tue, Feb 16, 2021 at 03:47:52PM +0530, Preeti Nagar wrote:
>> > The changes introduce a new security feature, RunTime Integrity Check
>> > (RTIC), designed to protect Linux Kernel at runtime. The motivation
>> > behind these changes is:
>> > 1. The system protection offered by Security Enhancements(SE) for
>> > Android relies on the assumption of kernel integrity. If the kernel
>> > itself is compromised (by a perhaps as yet unknown future vulnerability),
>> > SE for Android security mechanisms could potentially be disabled and
>> > rendered ineffective.
>> > 2. Qualcomm Snapdragon devices use Secure Boot, which adds cryptographic
>> > checks to each stage of the boot-up process, to assert the authenticity
>> > of all secure software images that the device executes.  However, due to
>> > various vulnerabilities in SW modules, the integrity of the system can be
>> > compromised at any time after device boot-up, leading to un-authorized
>> > SW executing.
>> >
>> > The feature's idea is to move some sensitive kernel structures to a
>> > separate page and monitor further any unauthorized changes to these,
>> > from higher Exception Levels using stage 2 MMU. Moving these to a
>> > different page will help avoid getting page faults from un-related data.
>> > The mechanism we have been working on removes the write permissions for
>> > HLOS in the stage 2 page tables for the regions to be monitored, such
>> > that any modification attempts to these will lead to faults being
>> > generated and handled by handlers. If the protected assets are moved to
>> > a separate page, faults will be generated corresponding to change attempts
>> > to these assets only. If not moved to a separate page, write attempts to
>> > un-related data present on the monitored pages will also be generated.
>> >
>> > Using this feature, some sensitive variables of the kernel which are
>> > initialized after init or are updated rarely can also be protected from
>> > simple overwrites and attacks trying to modify these.
>> 
>> Although I really like the idea of using stage-2 to protect the 
>> kernel, I
>> think the approach you outline here is deeply flawed. Identifying 
>> "sensitive
>> variables" of the kernel to protect is subjective and doesn't scale.
>> Furthermore, the triaging of what constitues a valid access is notably
>> absent from your description and is assumedly implemented in an opaque 
>> blob
>> at EL2.
>> 
>> I think a better approach would be along the lines of:
>> 
>>   1. Introduce the protection at stage-1 (like we already have for 
>> mapping
>>      e.g. the kernel text R/O)
>> 
>>   2. Implement the handlers in the kernel, so the heuristics are 
>> clear.
>> 
>>   3. Extend this to involve KVM, so that the host can manage its own
>>      stage-2 to firm-up the stage-1 protections.
>> 
> 
> Agree here. Making an arbitrary set of data structures r/o behind the
> OS's back doesn't seem like an easy thing to maintain or reason about,
> especially if this r/o-ness is only enforced on a tiny subset of
> devices. If something needs to be writable only at boot, we have
> __ro_after_init, and having hypervisor assisted enforcement of /that/
> might be a worthwhile thing to consider, including perhaps ways to do
> controlled patching of this region at runtime.
> 

Thank you for the suggestions. We will look into the possibility of 
protection
of __ro_after_init and controlled updates to these. I understand, if 
this can be
made generic as Will also suggested, it might be more useful and easy to 
scale
and maintain.

>> I also think we should avoid tying this to specific data structures.
>> Rather, we should introduce a mechanism to make arbitrary data 
>> read-only.
>> 
>> I've CC'd Ard and Marc, as I think they've both been thinking about 
>> this
>> sort of thing recently as well.
>> 
>> Will

Powered by blists - more mailing lists