Message-ID: <7f33036270f781568858bbc17a496b48@codeaurora.org>
Date:   Mon, 18 Jan 2021 11:26:24 +0530
From:   pnagar@...eaurora.org
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     arnd@...db.de, jmorris@...ei.org, 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@...r.kernel.org,
        psodagud@...eaurora.org, nmardana@...eaurora.org,
        dsule@...eaurora.org, Joe Perches <joe@...ches.com>,
        Miguel Ojeda <ojeda@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] selinux: security: Move selinux_state to a
 separate page

On 2021-01-12 22:36, Casey Schaufler wrote:
> On 1/12/2021 1:36 AM, pnagar@...eaurora.org wrote:
>> On 2021-01-08 22:41, Casey Schaufler wrote:
>>> On 1/8/2021 1:49 AM, 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 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.
>>> 
>>> It would be helpful if you characterized the "various 
>>> vulnerabilities"
>>> rather than simply asserting their existence. This would allow the 
>>> reviewer
>>> to determine if the proposed patch addresses the issue.
>>> 
>> There might not currently be vulnerabilities, but the system is meant 
>> more
>> specifically to harden valuable assets against future compromises. The 
>> key
>> value add is a third party independent entity keeping a watch on 
>> crucial
>> kernel assets.
> 
> Could you characterize the potential vulnerabilities, then?
> Seriously, there's a gazillion ways data integrity can be
> compromised. Which of those are addressed?
> 
1. Memory Corruption vulnerabilities (example buffer overflows) which 
can be
exploited to modify the critical kernel assets as a part of a directed 
attack
or inadvertently are the major type of vulnerabilities which this 
mechanism
directly addresses.
2. This can be useful in preventing privilege escalation attacks 
(modifying
some critical kernel structures) which can be caused by exploits 
targetting
various other types of vulnerabilities such as improper input validation 
and
integer overflows.

>> 
>>>> Using this mechanism, 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.
>>> 
>>> How would this interact with or complement __read_mostly?
>>> 
>> Currently, the mechanism we are working on developing is
>> independent of __read_mostly. This is something we can look more into
>> while working further on the mechanism.
> 
> Please either integrate the two or explain how they differ.
> It appears that you haven't considered how you might exploit
> or expand the existing mechanism.
> 
On looking up more about __read_mostly and also as David Howells shared, 
I
understand there two are different.
__read_mostly is seemed to be primarily designed for cache-related 
performance
improvement [1]. In RTIC design, the idea is of protection and 
monitoring aspect.
If there are some security-critical kernel assets which are expected to 
be
updated sometimes or rarely, then, we can monitor the writes to these as 
well
and check for any unauthorized attempts.
1. https://lkml.org/lkml/2007/12/13/487

>> 
>>>> 
>>>> Currently, the change moves selinux_state structure to a separate 
>>>> page. In
>>>> future we plan to move more security-related kernel assets to this 
>>>> page to
>>>> enhance protection.
>>> 
>>> What's special about selinux_state? What about the SELinux policy?
>>> How would I, as maintainer of the Smack security module, know if
>>> some Smack data should be treated the same way?
>>> 
>> We are investigating more of the SELinux related and other kernel 
>> assets
>> which can be included in the protection. The basis of selinux_state is
>> because disabling of SELinux is one of the common attack vectors in
>> Android. We understand any kernel assets, unauthorized changes to 
>> which
>> can give way to security or any other type of attack can be considered 
>> to
>> be a potential asset to be added to the protection.
> 
> Yeah, I get that. It looks like this could be a useful mechanism
> beyond SELinux. No point in hoarding it.
> 
Thank you! Will try and update the commit header line if possible to 
convey this
information.

>> 
>>>> 
>>>> We want to seek your suggestions and comments on the idea and the 
>>>> changes
>>>> in the patch.
>>>> 
>>>> Signed-off-by: Preeti Nagar <pnagar@...eaurora.org>
>>>> ---
>>>>  include/asm-generic/vmlinux.lds.h | 10 ++++++++++
>>>>  include/linux/init.h              |  4 ++++
>>>>  security/Kconfig                  | 10 ++++++++++
>>>>  security/selinux/hooks.c          |  4 ++++
>>>>  4 files changed, 28 insertions(+)
>>>> 
>>>> diff --git a/include/asm-generic/vmlinux.lds.h 
>>>> b/include/asm-generic/vmlinux.lds.h
>>>> index b2b3d81..158dbc2 100644
>>>> --- a/include/asm-generic/vmlinux.lds.h
>>>> +++ b/include/asm-generic/vmlinux.lds.h
>>>> @@ -770,6 +770,15 @@
>>>>          *(.scommon)                        \
>>>>      }
>>>> 
>>>> +#ifdef CONFIG_SECURITY_RTIC
>>>> +#define RTIC_BSS                            \
>>>> +    . = ALIGN(PAGE_SIZE);                        \
>>>> +    KEEP(*(.bss.rtic))                        \
>>>> +    . = ALIGN(PAGE_SIZE);
>>>> +#else
>>>> +#define RTIC_BSS
>>>> +#endif
>>>> +
>>>>  /*
>>>>   * Allow archectures to redefine BSS_FIRST_SECTIONS to add extra
>>>>   * sections to the front of bss.
>>>> @@ -782,6 +791,7 @@
>>>>      . = ALIGN(bss_align);                        \
>>>>      .bss : AT(ADDR(.bss) - LOAD_OFFSET) {                \
>>>>          BSS_FIRST_SECTIONS                    \
>>>> +        RTIC_BSS                        \
>>>>          . = ALIGN(PAGE_SIZE);                    \
>>>>          *(.bss..page_aligned)                    \
>>>>          . = ALIGN(PAGE_SIZE);                    \
>>>> diff --git a/include/linux/init.h b/include/linux/init.h
>>>> index 7b53cb3..617adcf 100644
>>>> --- a/include/linux/init.h
>>>> +++ b/include/linux/init.h
>>>> @@ -300,6 +300,10 @@ void __init parse_early_options(char *cmdline);
>>>>  /* Data marked not to be saved by software suspend */
>>>>  #define __nosavedata __section(".data..nosave")
>>>> 
>>>> +#ifdef CONFIG_SECURITY_RTIC
>>>> +#define __rticdata  __section(".bss.rtic")
>>>> +#endif
>>>> +
>>>>  #ifdef MODULE
>>>>  #define __exit_p(x) x
>>>>  #else
>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>> index 7561f6f..66b61b9 100644
>>>> --- a/security/Kconfig
>>>> +++ b/security/Kconfig
>>>> @@ -291,5 +291,15 @@ config LSM
>>>> 
>>>>  source "security/Kconfig.hardening"
>>>> 
>>>> +config SECURITY_RTIC
>>>> +        bool "RunTime Integrity Check feature"
>>> 
>>> Shouldn't this depend on the architecture(s) supporting the
>>> feature?
>>> 
>>>> +        help
>>>> +      RTIC(RunTime Integrity Check) feature is to protect Linux 
>>>> kernel
>>>> +      at runtime. This relocates some of the security sensitive 
>>>> kernel
>>>> +      structures to a separate page aligned special section.
>>>> +
>>>> +      This is to enable monitoring and protection of these kernel 
>>>> assets
>>>> +      from a higher exception level(EL) against any unauthorized 
>>>> changes.
>>> 
>>> "if you are unsure ..."
>>> 
>> We just thought keeping it generic might be a better idea, thus, moved 
>> the
>> changes to generic files from arch-specific files and thus, kept 
>> config also
>> independent of the arch. Can surely make this config arch dependent if 
>> that is
>> a better approach?
> 
> It's kind of silly to enable this if the hardware doesn't
> support it, isn't it?
> 
Yes, that makes sense. Will update depends on to the ARM64 arch in the 
next
version. Thank you.

Powered by blists - more mailing lists