[v10,8/8] selinux: include a consumer of the new IMA critical data hook

Commit Message

Tushar Sugandhi Jan. 8, 2021, 4:07 a.m. UTC

From: Lakshmi Ramasubramanian <[e-mail]>

SELinux stores the active policy in memory, so the changes to this data
at runtime would have an impact on the security guarantees provided
by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
provides a secure way for the attestation service to remotely validate
the policy contents at runtime.

Measure the hash of the loaded policy by calling the IMA hook
ima_measure_critical_data().  Since the size of the loaded policy
can be large (several MB), measure the hash of the policy instead of
the entire policy to avoid bloating the IMA log entry.

To enable SELinux data measurement, the following steps are required:

1, Add "ima_policy=critical_data" to the kernel command line arguments
   to enable measuring SELinux data at boot time.
For example,
  BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data

2, Add the following rule to /etc/ima/ima-policy
   measure func=CRITICAL_DATA label=selinux

Sample measurement of the hash of SELinux policy:

To verify the measured data with the current SELinux policy run
the following commands and verify the output hash values match.

  sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1

  grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6

Note that the actual verification of SELinux policy would require loading
the expected policy into an identical kernel on a pristine/known-safe
system and run the sha256sum /sys/kernel/selinux/policy there to get
the expected hash.

Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
Suggested-by: Stephen Smalley <[e-mail]>
Reviewed-by: Tyler Hicks <[e-mail]>
---
 Documentation/ABI/testing/ima_policy |  3 +-
 security/selinux/Makefile            |  2 +
 security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
 security/selinux/include/ima.h       | 24 +++++++++++
 security/selinux/include/security.h  |  3 +-
 security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
 6 files changed, 149 insertions(+), 11 deletions(-)
 create mode 100644 security/selinux/ima.c
 create mode 100644 security/selinux/include/ima.h

Comments

Paul Moore Jan. 12, 2021, 4:27 p.m. UTC | #1

On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
<[e-mail]> wrote:
> From: Lakshmi Ramasubramanian <[e-mail]>
>
> SELinux stores the active policy in memory, so the changes to this data
> at runtime would have an impact on the security guarantees provided
> by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> provides a secure way for the attestation service to remotely validate
> the policy contents at runtime.
>
> Measure the hash of the loaded policy by calling the IMA hook
> ima_measure_critical_data().  Since the size of the loaded policy
> can be large (several MB), measure the hash of the policy instead of
> the entire policy to avoid bloating the IMA log entry.
>
> To enable SELinux data measurement, the following steps are required:
>
> 1, Add "ima_policy=critical_data" to the kernel command line arguments
>    to enable measuring SELinux data at boot time.
> For example,
>   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
>
> 2, Add the following rule to /etc/ima/ima-policy
>    measure func=CRITICAL_DATA label=selinux
>
> Sample measurement of the hash of SELinux policy:
>
> To verify the measured data with the current SELinux policy run
> the following commands and verify the output hash values match.
>
>   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
>
>   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
>
> Note that the actual verification of SELinux policy would require loading
> the expected policy into an identical kernel on a pristine/known-safe
> system and run the sha256sum /sys/kernel/selinux/policy there to get
> the expected hash.
>
> Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> Suggested-by: Stephen Smalley <[e-mail]>
> Reviewed-by: Tyler Hicks <[e-mail]>
> ---
>  Documentation/ABI/testing/ima_policy |  3 +-
>  security/selinux/Makefile            |  2 +
>  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
>  security/selinux/include/ima.h       | 24 +++++++++++
>  security/selinux/include/security.h  |  3 +-
>  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
>  6 files changed, 149 insertions(+), 11 deletions(-)
>  create mode 100644 security/selinux/ima.c
>  create mode 100644 security/selinux/include/ima.h

I remain concerned about the possibility of bypassing a measurement by
tampering with the time, but I appear to be the only one who is
worried about this so I'm not going to block this patch on those
grounds.

Acked-by: Paul Moore <[e-mail]>

> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 54fe1c15ed50..8365596cb42b 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -52,8 +52,9 @@ Description:
>                         template:= name of a defined IMA template type
>                         (eg, ima-ng). Only valid when action is "measure".
>                         pcr:= decimal value
> -                       label:= [data_label]
> +                       label:= [selinux]|[data_label]
>                         data_label:= a unique string used for grouping and limiting critical data.
> +                       For example, "selinux" to measure critical data for SELinux.
>
>                   default policy:
>                         # PROC_SUPER_MAGIC
> diff --git a/security/selinux/Makefile b/security/selinux/Makefile
> index 4d8e0e8adf0b..776162444882 100644
> --- a/security/selinux/Makefile
> +++ b/security/selinux/Makefile
> @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o
>
>  selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o
>
> +selinux-$(CONFIG_IMA) += ima.o
> +
>  ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include
>
>  $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h
> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> new file mode 100644
> index 000000000000..0b835bdc3aa9
> --- /dev/null
> +++ b/security/selinux/ima.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Microsoft Corporation
> + *
> + * Author: Lakshmi Ramasubramanian ([e-mail])
> + *
> + * Measure critical data structures maintainted by SELinux
> + * using IMA subsystem.
> + */
> +#include <linux/vmalloc.h>
> +#include <linux/ktime.h>
> +#include <linux/ima.h>
> +#include "security.h"
> +#include "ima.h"
> +
> +/*
> + * selinux_ima_measure_state - Measure hash of the SELinux policy
> + *
> + * @state: selinux state struct
> + *
> + * NOTE: This function must be called with policy_mutex held.
> + */
> +void selinux_ima_measure_state(struct selinux_state *state)
> +{
> +       struct timespec64 cur_time;
> +       void *policy = NULL;
> +       char *policy_event_name = NULL;
> +       size_t policy_len;
> +       int rc = 0;
> +
> +       /*
> +        * Measure SELinux policy only after initialization is completed.
> +        */
> +       if (!selinux_initialized(state))
> +               return;
> +
> +       /*
> +        * Pass a unique "event_name" to the IMA hook so that IMA subsystem
> +        * will always measure the given data.
> +        */
> +       ktime_get_real_ts64(&cur_time);
> +       policy_event_name = kasprintf(GFP_KERNEL, "%s-%lld:%09ld",
> +                                     "selinux-policy-hash",
> +                                     cur_time.tv_sec, cur_time.tv_nsec);
> +       if (!policy_event_name) {
> +               pr_err("SELinux: %s: event name for policy not allocated.\n",
> +                      __func__);
> +               goto out;
> +       }
> +
> +       rc = security_read_state_kernel(state, &policy, &policy_len);
> +       if (rc) {
> +               pr_err("SELinux: %s: failed to read policy %d.\n", __func__, rc);
> +               goto out;
> +       }
> +
> +       ima_measure_critical_data("selinux", policy_event_name,
> +                                 policy, policy_len, true);
> +
> +       vfree(policy);
> +
> +out:
> +       kfree(policy_event_name);
> +}
> diff --git a/security/selinux/include/ima.h b/security/selinux/include/ima.h
> new file mode 100644
> index 000000000000..d69c36611423
> --- /dev/null
> +++ b/security/selinux/include/ima.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2021 Microsoft Corporation
> + *
> + * Author: Lakshmi Ramasubramanian ([e-mail])
> + *
> + * Measure critical data structures maintainted by SELinux
> + * using IMA subsystem.
> + */
> +
> +#ifndef _SELINUX_IMA_H_
> +#define _SELINUX_IMA_H_
> +
> +#include "security.h"
> +
> +#ifdef CONFIG_IMA
> +extern void selinux_ima_measure_state(struct selinux_state *selinux_state);
> +#else
> +static inline void selinux_ima_measure_state(struct selinux_state *selinux_state)
> +{
> +}
> +#endif
> +
> +#endif /* _SELINUX_IMA_H_ */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 3cc8bab31ea8..29cae32d3fc5 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -229,7 +229,8 @@ void selinux_policy_cancel(struct selinux_state *state,
>                         struct selinux_policy *policy);
>  int security_read_policy(struct selinux_state *state,
>                          void **data, size_t *len);
> -
> +int security_read_state_kernel(struct selinux_state *state,
> +                              void **data, size_t *len);
>  int security_policycap_supported(struct selinux_state *state,
>                                  unsigned int req_cap);
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 9704c8a32303..cc8dbc4ed8db 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -65,6 +65,7 @@
>  #include "ebitmap.h"
>  #include "audit.h"
>  #include "policycap_names.h"
> +#include "ima.h"
>
>  /* Forward declaration. */
>  static int context_struct_to_string(struct policydb *policydb,
> @@ -2180,6 +2181,7 @@ static void selinux_notify_policy_change(struct selinux_state *state,
>         selinux_status_update_policyload(state, seqno);
>         selinux_netlbl_cache_invalidate();
>         selinux_xfrm_notify_policyload();
> +       selinux_ima_measure_state(state);
>  }
>
>  void selinux_policy_commit(struct selinux_state *state,
> @@ -3875,8 +3877,33 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>  }
>  #endif /* CONFIG_NETLABEL */
>
> +/**
> + * __security_read_policy - read the policy.
> + * @policy: SELinux policy
> + * @data: binary policy data
> + * @len: length of data in bytes
> + *
> + */
> +static int __security_read_policy(struct selinux_policy *policy,
> +                                 void *data, size_t *len)
> +{
> +       int rc;
> +       struct policy_file fp;
> +
> +       fp.data = data;
> +       fp.len = *len;
> +
> +       rc = policydb_write(&policy->policydb, &fp);
> +       if (rc)
> +               return rc;
> +
> +       *len = (unsigned long)fp.data - (unsigned long)data;
> +       return 0;
> +}
> +
>  /**
>   * security_read_policy - read the policy.
> + * @state: selinux_state
>   * @data: binary policy data
>   * @len: length of data in bytes
>   *
> @@ -3885,8 +3912,6 @@ int security_read_policy(struct selinux_state *state,
>                          void **data, size_t *len)
>  {
>         struct selinux_policy *policy;
> -       int rc;
> -       struct policy_file fp;
>
>         policy = rcu_dereference_protected(
>                         state->policy, lockdep_is_held(&state->policy_mutex));
> @@ -3898,14 +3923,35 @@ int security_read_policy(struct selinux_state *state,
>         if (!*data)
>                 return -ENOMEM;
>
> -       fp.data = *data;
> -       fp.len = *len;
> +       return __security_read_policy(policy, *data, len);
> +}
>
> -       rc = policydb_write(&policy->policydb, &fp);
> -       if (rc)
> -               return rc;
> +/**
> + * security_read_state_kernel - read the policy.
> + * @state: selinux_state
> + * @data: binary policy data
> + * @len: length of data in bytes
> + *
> + * Allocates kernel memory for reading SELinux policy.
> + * This function is for internal use only and should not
> + * be used for returning data to user space.
> + *
> + * This function must be called with policy_mutex held.
> + */
> +int security_read_state_kernel(struct selinux_state *state,
> +                              void **data, size_t *len)
> +{
> +       struct selinux_policy *policy;
>
> -       *len = (unsigned long)fp.data - (unsigned long)*data;
> -       return 0;
> +       policy = rcu_dereference_protected(
> +                       state->policy, lockdep_is_held(&state->policy_mutex));
> +       if (!policy)
> +               return -EINVAL;
> +
> +       *len = policy->policydb.len;
> +       *data = vmalloc(*len);
> +       if (!*data)
> +               return -ENOMEM;
>
> +       return __security_read_policy(policy, *data, len);
>  }
> --
> 2.17.1
>

Lakshmi Ramasubramanian Jan. 12, 2021, 6:25 p.m. UTC | #2

On 1/12/21 8:27 AM, Paul Moore wrote:
> On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> <[e-mail]> wrote:
>> From: Lakshmi Ramasubramanian <[e-mail]>
>>
>> SELinux stores the active policy in memory, so the changes to this data
>> at runtime would have an impact on the security guarantees provided
>> by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
>> provides a secure way for the attestation service to remotely validate
>> the policy contents at runtime.
>>
>> Measure the hash of the loaded policy by calling the IMA hook
>> ima_measure_critical_data().  Since the size of the loaded policy
>> can be large (several MB), measure the hash of the policy instead of
>> the entire policy to avoid bloating the IMA log entry.
>>
>> To enable SELinux data measurement, the following steps are required:
>>
>> 1, Add "ima_policy=critical_data" to the kernel command line arguments
>>     to enable measuring SELinux data at boot time.
>> For example,
>>    BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
>>
>> 2, Add the following rule to /etc/ima/ima-policy
>>     measure func=CRITICAL_DATA label=selinux
>>
>> Sample measurement of the hash of SELinux policy:
>>
>> To verify the measured data with the current SELinux policy run
>> the following commands and verify the output hash values match.
>>
>>    sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
>>
>>    grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
>>
>> Note that the actual verification of SELinux policy would require loading
>> the expected policy into an identical kernel on a pristine/known-safe
>> system and run the sha256sum /sys/kernel/selinux/policy there to get
>> the expected hash.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
>> Suggested-by: Stephen Smalley <[e-mail]>
>> Reviewed-by: Tyler Hicks <[e-mail]>
>> ---
>>   Documentation/ABI/testing/ima_policy |  3 +-
>>   security/selinux/Makefile            |  2 +
>>   security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
>>   security/selinux/include/ima.h       | 24 +++++++++++
>>   security/selinux/include/security.h  |  3 +-
>>   security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
>>   6 files changed, 149 insertions(+), 11 deletions(-)
>>   create mode 100644 security/selinux/ima.c
>>   create mode 100644 security/selinux/include/ima.h
> 
> I remain concerned about the possibility of bypassing a measurement by
> tampering with the time, but I appear to be the only one who is
> worried about this so I'm not going to block this patch on those
> grounds.
> 
> Acked-by: Paul Moore <[e-mail]>

Thanks Paul.

  -lakshmi

Mimi Zohar Jan. 13, 2021, 7:13 p.m. UTC | #3

On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> <[e-mail]> wrote:
> > From: Lakshmi Ramasubramanian <[e-mail]>
> >
> > SELinux stores the active policy in memory, so the changes to this data
> > at runtime would have an impact on the security guarantees provided
> > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > provides a secure way for the attestation service to remotely validate
> > the policy contents at runtime.
> >
> > Measure the hash of the loaded policy by calling the IMA hook
> > ima_measure_critical_data().  Since the size of the loaded policy
> > can be large (several MB), measure the hash of the policy instead of
> > the entire policy to avoid bloating the IMA log entry.
> >
> > To enable SELinux data measurement, the following steps are required:
> >
> > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> >    to enable measuring SELinux data at boot time.
> > For example,
> >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> >
> > 2, Add the following rule to /etc/ima/ima-policy
> >    measure func=CRITICAL_DATA label=selinux
> >
> > Sample measurement of the hash of SELinux policy:
> >
> > To verify the measured data with the current SELinux policy run
> > the following commands and verify the output hash values match.
> >
> >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> >
> >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> >
> > Note that the actual verification of SELinux policy would require loading
> > the expected policy into an identical kernel on a pristine/known-safe
> > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > the expected hash.
> >
> > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > Suggested-by: Stephen Smalley <[e-mail]>
> > Reviewed-by: Tyler Hicks <[e-mail]>
> > ---
> >  Documentation/ABI/testing/ima_policy |  3 +-
> >  security/selinux/Makefile            |  2 +
> >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> >  security/selinux/include/ima.h       | 24 +++++++++++
> >  security/selinux/include/security.h  |  3 +-
> >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> >  6 files changed, 149 insertions(+), 11 deletions(-)
> >  create mode 100644 security/selinux/ima.c
> >  create mode 100644 security/selinux/include/ima.h
> 
> I remain concerned about the possibility of bypassing a measurement by
> tampering with the time, but I appear to be the only one who is
> worried about this so I'm not going to block this patch on those
> grounds.
> 
> Acked-by: Paul Moore <[e-mail]>

Thanks, Paul.

Including any unique string would cause the buffer hash to change,
forcing a new measurement.  Perhaps they were concerned with
overflowing a counter.

Mimi

> > +/*
> > + * selinux_ima_measure_state - Measure hash of the SELinux policy
> > + *
> > + * @state: selinux state struct
> > + *
> > + * NOTE: This function must be called with policy_mutex held.
> > + */
> > +void selinux_ima_measure_state(struct selinux_state *state)
> > +{
> > +       struct timespec64 cur_time;
> > +       void *policy = NULL;
> > +       char *policy_event_name = NULL;
> > +       size_t policy_len;
> > +       int rc = 0;
> > +
> > +       /*
> > +        * Measure SELinux policy only after initialization is completed.
> > +        */
> > +       if (!selinux_initialized(state))
> > +               return;
> > +
> > +       /*
> > +        * Pass a unique "event_name" to the IMA hook so that IMA subsystem
> > +        * will always measure the given data.
> > +        */
> > +       ktime_get_real_ts64(&cur_time);
> > +       policy_event_name = kasprintf(GFP_KERNEL, "%s-%lld:%09ld",
> > +                                     "selinux-policy-hash",
> > +                                     cur_time.tv_sec, cur_time.tv_nsec);
> > +       if (!policy_event_name) {
> > +               pr_err("SELinux: %s: event name for policy not allocated.\n",
> > +                      __func__);
> > +               goto out;
> > +       }
> > +

Paul Moore Jan. 13, 2021, 7:19 p.m. UTC | #4

On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > <[e-mail]> wrote:
> > > From: Lakshmi Ramasubramanian <[e-mail]>
> > >
> > > SELinux stores the active policy in memory, so the changes to this data
> > > at runtime would have an impact on the security guarantees provided
> > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > provides a secure way for the attestation service to remotely validate
> > > the policy contents at runtime.
> > >
> > > Measure the hash of the loaded policy by calling the IMA hook
> > > ima_measure_critical_data().  Since the size of the loaded policy
> > > can be large (several MB), measure the hash of the policy instead of
> > > the entire policy to avoid bloating the IMA log entry.
> > >
> > > To enable SELinux data measurement, the following steps are required:
> > >
> > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > >    to enable measuring SELinux data at boot time.
> > > For example,
> > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > >
> > > 2, Add the following rule to /etc/ima/ima-policy
> > >    measure func=CRITICAL_DATA label=selinux
> > >
> > > Sample measurement of the hash of SELinux policy:
> > >
> > > To verify the measured data with the current SELinux policy run
> > > the following commands and verify the output hash values match.
> > >
> > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > >
> > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > >
> > > Note that the actual verification of SELinux policy would require loading
> > > the expected policy into an identical kernel on a pristine/known-safe
> > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > the expected hash.
> > >
> > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > Suggested-by: Stephen Smalley <[e-mail]>
> > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > ---
> > >  Documentation/ABI/testing/ima_policy |  3 +-
> > >  security/selinux/Makefile            |  2 +
> > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > >  security/selinux/include/ima.h       | 24 +++++++++++
> > >  security/selinux/include/security.h  |  3 +-
> > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > >  create mode 100644 security/selinux/ima.c
> > >  create mode 100644 security/selinux/include/ima.h
> >
> > I remain concerned about the possibility of bypassing a measurement by
> > tampering with the time, but I appear to be the only one who is
> > worried about this so I'm not going to block this patch on those
> > grounds.
> >
> > Acked-by: Paul Moore <[e-mail]>
>
> Thanks, Paul.
>
> Including any unique string would cause the buffer hash to change,
> forcing a new measurement.  Perhaps they were concerned with
> overflowing a counter.

My understanding is that Lakshmi wanted to force a new measurement
each time and felt using a timestamp would be the best way to do that.
A counter, even if it wraps, would have a different value each time
whereas a timestamp is vulnerable to time adjustments.  While a
properly controlled and audited system could be configured and
monitored to detect such an event (I *think*), why rely on that if it
isn't necessary?

Mimi Zohar Jan. 13, 2021, 9:11 p.m. UTC | #5

On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote:
> On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > > <[e-mail]> wrote:
> > > > From: Lakshmi Ramasubramanian <[e-mail]>
> > > >
> > > > SELinux stores the active policy in memory, so the changes to this data
> > > > at runtime would have an impact on the security guarantees provided
> > > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > > provides a secure way for the attestation service to remotely validate
> > > > the policy contents at runtime.
> > > >
> > > > Measure the hash of the loaded policy by calling the IMA hook
> > > > ima_measure_critical_data().  Since the size of the loaded policy
> > > > can be large (several MB), measure the hash of the policy instead of
> > > > the entire policy to avoid bloating the IMA log entry.
> > > >
> > > > To enable SELinux data measurement, the following steps are required:
> > > >
> > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > > >    to enable measuring SELinux data at boot time.
> > > > For example,
> > > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > > >
> > > > 2, Add the following rule to /etc/ima/ima-policy
> > > >    measure func=CRITICAL_DATA label=selinux
> > > >
> > > > Sample measurement of the hash of SELinux policy:
> > > >
> > > > To verify the measured data with the current SELinux policy run
> > > > the following commands and verify the output hash values match.
> > > >
> > > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > > >
> > > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > > >
> > > > Note that the actual verification of SELinux policy would require loading
> > > > the expected policy into an identical kernel on a pristine/known-safe
> > > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > > the expected hash.
> > > >
> > > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > > Suggested-by: Stephen Smalley <[e-mail]>
> > > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > > ---
> > > >  Documentation/ABI/testing/ima_policy |  3 +-
> > > >  security/selinux/Makefile            |  2 +
> > > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > > >  security/selinux/include/ima.h       | 24 +++++++++++
> > > >  security/selinux/include/security.h  |  3 +-
> > > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > > >  create mode 100644 security/selinux/ima.c
> > > >  create mode 100644 security/selinux/include/ima.h
> > >
> > > I remain concerned about the possibility of bypassing a measurement by
> > > tampering with the time, but I appear to be the only one who is
> > > worried about this so I'm not going to block this patch on those
> > > grounds.
> > >
> > > Acked-by: Paul Moore <[e-mail]>
> >
> > Thanks, Paul.
> >
> > Including any unique string would cause the buffer hash to change,
> > forcing a new measurement.  Perhaps they were concerned with
> > overflowing a counter.
> 
> My understanding is that Lakshmi wanted to force a new measurement
> each time and felt using a timestamp would be the best way to do that.
> A counter, even if it wraps, would have a different value each time
> whereas a timestamp is vulnerable to time adjustments.  While a
> properly controlled and audited system could be configured and
> monitored to detect such an event (I *think*), why rely on that if it
> isn't necessary?

Why are you saying that even if the counter wraps a new measurement is
guaranteed.   I agree with the rest of what you said.

Mimi

Paul Moore Jan. 13, 2021, 10:10 p.m. UTC | #6

On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar <[e-mail]> wrote:
> On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote:
> > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > > > <[e-mail]> wrote:
> > > > > From: Lakshmi Ramasubramanian <[e-mail]>
> > > > >
> > > > > SELinux stores the active policy in memory, so the changes to this data
> > > > > at runtime would have an impact on the security guarantees provided
> > > > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > > > provides a secure way for the attestation service to remotely validate
> > > > > the policy contents at runtime.
> > > > >
> > > > > Measure the hash of the loaded policy by calling the IMA hook
> > > > > ima_measure_critical_data().  Since the size of the loaded policy
> > > > > can be large (several MB), measure the hash of the policy instead of
> > > > > the entire policy to avoid bloating the IMA log entry.
> > > > >
> > > > > To enable SELinux data measurement, the following steps are required:
> > > > >
> > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > > > >    to enable measuring SELinux data at boot time.
> > > > > For example,
> > > > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > > > >
> > > > > 2, Add the following rule to /etc/ima/ima-policy
> > > > >    measure func=CRITICAL_DATA label=selinux
> > > > >
> > > > > Sample measurement of the hash of SELinux policy:
> > > > >
> > > > > To verify the measured data with the current SELinux policy run
> > > > > the following commands and verify the output hash values match.
> > > > >
> > > > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > > > >
> > > > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > > > >
> > > > > Note that the actual verification of SELinux policy would require loading
> > > > > the expected policy into an identical kernel on a pristine/known-safe
> > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > > > the expected hash.
> > > > >
> > > > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > > > Suggested-by: Stephen Smalley <[e-mail]>
> > > > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > > > ---
> > > > >  Documentation/ABI/testing/ima_policy |  3 +-
> > > > >  security/selinux/Makefile            |  2 +
> > > > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > > > >  security/selinux/include/ima.h       | 24 +++++++++++
> > > > >  security/selinux/include/security.h  |  3 +-
> > > > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > > > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > > > >  create mode 100644 security/selinux/ima.c
> > > > >  create mode 100644 security/selinux/include/ima.h
> > > >
> > > > I remain concerned about the possibility of bypassing a measurement by
> > > > tampering with the time, but I appear to be the only one who is
> > > > worried about this so I'm not going to block this patch on those
> > > > grounds.
> > > >
> > > > Acked-by: Paul Moore <[e-mail]>
> > >
> > > Thanks, Paul.
> > >
> > > Including any unique string would cause the buffer hash to change,
> > > forcing a new measurement.  Perhaps they were concerned with
> > > overflowing a counter.
> >
> > My understanding is that Lakshmi wanted to force a new measurement
> > each time and felt using a timestamp would be the best way to do that.
> > A counter, even if it wraps, would have a different value each time
> > whereas a timestamp is vulnerable to time adjustments.  While a
> > properly controlled and audited system could be configured and
> > monitored to detect such an event (I *think*), why rely on that if it
> > isn't necessary?
>
> Why are you saying that even if the counter wraps a new measurement is
> guaranteed.   I agree with the rest of what you said.

I was assuming that the IMA code simply compares the passed
"policy_event_name" value to the previous value, if they are different
a new measurement is taken, if they are the same the measurement
request is ignored.  If this is the case the counter value is only
important in as much as that it is different from the previous value,
even simply toggling a single bit back and forth would suffice in this
case.  IMA doesn't keep a record of every previous "policy_event_name"
value does it?  Am I misunderstanding how
ima_measure_critical_data(...) works?

Mimi Zohar Jan. 13, 2021, 11:10 p.m. UTC | #7

On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote:
> On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar <[e-mail]> wrote:
> > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote:
> > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > > > > <[e-mail]> wrote:
> > > > > > From: Lakshmi Ramasubramanian <[e-mail]>
> > > > > >
> > > > > > SELinux stores the active policy in memory, so the changes to this data
> > > > > > at runtime would have an impact on the security guarantees provided
> > > > > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > > > > provides a secure way for the attestation service to remotely validate
> > > > > > the policy contents at runtime.
> > > > > >
> > > > > > Measure the hash of the loaded policy by calling the IMA hook
> > > > > > ima_measure_critical_data().  Since the size of the loaded policy
> > > > > > can be large (several MB), measure the hash of the policy instead of
> > > > > > the entire policy to avoid bloating the IMA log entry.
> > > > > >
> > > > > > To enable SELinux data measurement, the following steps are required:
> > > > > >
> > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > > > > >    to enable measuring SELinux data at boot time.
> > > > > > For example,
> > > > > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > > > > >
> > > > > > 2, Add the following rule to /etc/ima/ima-policy
> > > > > >    measure func=CRITICAL_DATA label=selinux
> > > > > >
> > > > > > Sample measurement of the hash of SELinux policy:
> > > > > >
> > > > > > To verify the measured data with the current SELinux policy run
> > > > > > the following commands and verify the output hash values match.
> > > > > >
> > > > > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > > > > >
> > > > > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > > > > >
> > > > > > Note that the actual verification of SELinux policy would require loading
> > > > > > the expected policy into an identical kernel on a pristine/known-safe
> > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > > > > the expected hash.
> > > > > >
> > > > > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > > > > Suggested-by: Stephen Smalley <[e-mail]>
> > > > > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > > > > ---
> > > > > >  Documentation/ABI/testing/ima_policy |  3 +-
> > > > > >  security/selinux/Makefile            |  2 +
> > > > > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > > > > >  security/selinux/include/ima.h       | 24 +++++++++++
> > > > > >  security/selinux/include/security.h  |  3 +-
> > > > > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > > > > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > > > > >  create mode 100644 security/selinux/ima.c
> > > > > >  create mode 100644 security/selinux/include/ima.h
> > > > >
> > > > > I remain concerned about the possibility of bypassing a measurement by
> > > > > tampering with the time, but I appear to be the only one who is
> > > > > worried about this so I'm not going to block this patch on those
> > > > > grounds.
> > > > >
> > > > > Acked-by: Paul Moore <[e-mail]>
> > > >
> > > > Thanks, Paul.
> > > >
> > > > Including any unique string would cause the buffer hash to change,
> > > > forcing a new measurement.  Perhaps they were concerned with
> > > > overflowing a counter.
> > >
> > > My understanding is that Lakshmi wanted to force a new measurement
> > > each time and felt using a timestamp would be the best way to do that.
> > > A counter, even if it wraps, would have a different value each time
> > > whereas a timestamp is vulnerable to time adjustments.  While a
> > > properly controlled and audited system could be configured and
> > > monitored to detect such an event (I *think*), why rely on that if it
> > > isn't necessary?
> >
> > Why are you saying that even if the counter wraps a new measurement is
> > guaranteed.   I agree with the rest of what you said.
> 
> I was assuming that the IMA code simply compares the passed
> "policy_event_name" value to the previous value, if they are different
> a new measurement is taken, if they are the same the measurement
> request is ignored.  If this is the case the counter value is only
> important in as much as that it is different from the previous value,
> even simply toggling a single bit back and forth would suffice in this
> case.  IMA doesn't keep a record of every previous "policy_event_name"
> value does it?  Am I misunderstanding how
> ima_measure_critical_data(...) works?

Originally, there was quite a bit of discussion as to how much or how
little should be measured for a number of reasons.  One reason is that
the TPM is relatively slow.  Another reason is to limit the size of the
measurement list.  For this reason, duplicate hashes aren't added to
the measurement list or extended into the TPM.

When a dentry is removed from cache, its also removed from IMA's iint
cache.  A subsequent file read would result in adding the measurement
and extending the TPM again.  ima_lookup_digest_entry() is called to
prevent adding the duplicate entry. 

Lakshmi is trying to address the situation where an event changes a
value, but then is restored to the original value.  The original and
subsequent events are measured, but restoring to the original value
isn't re-measured.  This isn't any different than when a file is
modified and then reverted.

Instead of changing the name like this, which doesn't work for files,
allowing duplicate measurements should be generic, based on policy.

Mimi

Paul Moore Jan. 14, 2021, 2:40 a.m. UTC | #8

On Wed, Jan 13, 2021 at 6:11 PM Mimi Zohar <[e-mail]> wrote:
> On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote:
> > On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar <[e-mail]> wrote:
> > > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote:
> > > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> > > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > > > > > <[e-mail]> wrote:
> > > > > > > From: Lakshmi Ramasubramanian <[e-mail]>
> > > > > > >
> > > > > > > SELinux stores the active policy in memory, so the changes to this data
> > > > > > > at runtime would have an impact on the security guarantees provided
> > > > > > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > > > > > provides a secure way for the attestation service to remotely validate
> > > > > > > the policy contents at runtime.
> > > > > > >
> > > > > > > Measure the hash of the loaded policy by calling the IMA hook
> > > > > > > ima_measure_critical_data().  Since the size of the loaded policy
> > > > > > > can be large (several MB), measure the hash of the policy instead of
> > > > > > > the entire policy to avoid bloating the IMA log entry.
> > > > > > >
> > > > > > > To enable SELinux data measurement, the following steps are required:
> > > > > > >
> > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > > > > > >    to enable measuring SELinux data at boot time.
> > > > > > > For example,
> > > > > > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > > > > > >
> > > > > > > 2, Add the following rule to /etc/ima/ima-policy
> > > > > > >    measure func=CRITICAL_DATA label=selinux
> > > > > > >
> > > > > > > Sample measurement of the hash of SELinux policy:
> > > > > > >
> > > > > > > To verify the measured data with the current SELinux policy run
> > > > > > > the following commands and verify the output hash values match.
> > > > > > >
> > > > > > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > > > > > >
> > > > > > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > > > > > >
> > > > > > > Note that the actual verification of SELinux policy would require loading
> > > > > > > the expected policy into an identical kernel on a pristine/known-safe
> > > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > > > > > the expected hash.
> > > > > > >
> > > > > > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > > > > > Suggested-by: Stephen Smalley <[e-mail]>
> > > > > > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > > > > > ---
> > > > > > >  Documentation/ABI/testing/ima_policy |  3 +-
> > > > > > >  security/selinux/Makefile            |  2 +
> > > > > > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > > > > > >  security/selinux/include/ima.h       | 24 +++++++++++
> > > > > > >  security/selinux/include/security.h  |  3 +-
> > > > > > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > > > > > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > > > > > >  create mode 100644 security/selinux/ima.c
> > > > > > >  create mode 100644 security/selinux/include/ima.h
> > > > > >
> > > > > > I remain concerned about the possibility of bypassing a measurement by
> > > > > > tampering with the time, but I appear to be the only one who is
> > > > > > worried about this so I'm not going to block this patch on those
> > > > > > grounds.
> > > > > >
> > > > > > Acked-by: Paul Moore <[e-mail]>
> > > > >
> > > > > Thanks, Paul.
> > > > >
> > > > > Including any unique string would cause the buffer hash to change,
> > > > > forcing a new measurement.  Perhaps they were concerned with
> > > > > overflowing a counter.
> > > >
> > > > My understanding is that Lakshmi wanted to force a new measurement
> > > > each time and felt using a timestamp would be the best way to do that.
> > > > A counter, even if it wraps, would have a different value each time
> > > > whereas a timestamp is vulnerable to time adjustments.  While a
> > > > properly controlled and audited system could be configured and
> > > > monitored to detect such an event (I *think*), why rely on that if it
> > > > isn't necessary?
> > >
> > > Why are you saying that even if the counter wraps a new measurement is
> > > guaranteed.   I agree with the rest of what you said.
> >
> > I was assuming that the IMA code simply compares the passed
> > "policy_event_name" value to the previous value, if they are different
> > a new measurement is taken, if they are the same the measurement
> > request is ignored.  If this is the case the counter value is only
> > important in as much as that it is different from the previous value,
> > even simply toggling a single bit back and forth would suffice in this
> > case.  IMA doesn't keep a record of every previous "policy_event_name"
> > value does it?  Am I misunderstanding how
> > ima_measure_critical_data(...) works?
>
> Originally, there was quite a bit of discussion as to how much or how
> little should be measured for a number of reasons.  One reason is that
> the TPM is relatively slow.  Another reason is to limit the size of the
> measurement list.  For this reason, duplicate hashes aren't added to
> the measurement list or extended into the TPM.
>
> When a dentry is removed from cache, its also removed from IMA's iint
> cache.  A subsequent file read would result in adding the measurement
> and extending the TPM again.  ima_lookup_digest_entry() is called to
> prevent adding the duplicate entry.
>
> Lakshmi is trying to address the situation where an event changes a
> value, but then is restored to the original value.  The original and
> subsequent events are measured, but restoring to the original value
> isn't re-measured.  This isn't any different than when a file is
> modified and then reverted.
>
> Instead of changing the name like this, which doesn't work for files,
> allowing duplicate measurements should be generic, based on policy.

Perhaps it is just the end of the day and I'm a bit tired, but I just
read all of the above and I have no idea what your current thoughts
are regarding this patch.

Mimi Zohar Jan. 14, 2021, 2:49 a.m. UTC | #9

On Wed, 2021-01-13 at 21:40 -0500, Paul Moore wrote:
> On Wed, Jan 13, 2021 at 6:11 PM Mimi Zohar <[e-mail]> wrote:
> > On Wed, 2021-01-13 at 17:10 -0500, Paul Moore wrote:
> > > On Wed, Jan 13, 2021 at 4:11 PM Mimi Zohar <[e-mail]> wrote:
> > > > On Wed, 2021-01-13 at 14:19 -0500, Paul Moore wrote:
> > > > > On Wed, Jan 13, 2021 at 2:13 PM Mimi Zohar <[e-mail]> wrote:
> > > > > > On Tue, 2021-01-12 at 11:27 -0500, Paul Moore wrote:
> > > > > > > On Thu, Jan 7, 2021 at 11:07 PM Tushar Sugandhi
> > > > > > > <[e-mail]> wrote:
> > > > > > > > From: Lakshmi Ramasubramanian <[e-mail]>
> > > > > > > >
> > > > > > > > SELinux stores the active policy in memory, so the changes to this data
> > > > > > > > at runtime would have an impact on the security guarantees provided
> > > > > > > > by SELinux.  Measuring in-memory SELinux policy through IMA subsystem
> > > > > > > > provides a secure way for the attestation service to remotely validate
> > > > > > > > the policy contents at runtime.
> > > > > > > >
> > > > > > > > Measure the hash of the loaded policy by calling the IMA hook
> > > > > > > > ima_measure_critical_data().  Since the size of the loaded policy
> > > > > > > > can be large (several MB), measure the hash of the policy instead of
> > > > > > > > the entire policy to avoid bloating the IMA log entry.
> > > > > > > >
> > > > > > > > To enable SELinux data measurement, the following steps are required:
> > > > > > > >
> > > > > > > > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > > > > > > >    to enable measuring SELinux data at boot time.
> > > > > > > > For example,
> > > > > > > >   BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> > > > > > > >
> > > > > > > > 2, Add the following rule to /etc/ima/ima-policy
> > > > > > > >    measure func=CRITICAL_DATA label=selinux
> > > > > > > >
> > > > > > > > Sample measurement of the hash of SELinux policy:
> > > > > > > >
> > > > > > > > To verify the measured data with the current SELinux policy run
> > > > > > > > the following commands and verify the output hash values match.
> > > > > > > >
> > > > > > > >   sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> > > > > > > >
> > > > > > > >   grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> > > > > > > >
> > > > > > > > Note that the actual verification of SELinux policy would require loading
> > > > > > > > the expected policy into an identical kernel on a pristine/known-safe
> > > > > > > > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > > > > > > > the expected hash.
> > > > > > > >
> > > > > > > > Signed-off-by: Lakshmi Ramasubramanian <[e-mail]>
> > > > > > > > Suggested-by: Stephen Smalley <[e-mail]>
> > > > > > > > Reviewed-by: Tyler Hicks <[e-mail]>
> > > > > > > > ---
> > > > > > > >  Documentation/ABI/testing/ima_policy |  3 +-
> > > > > > > >  security/selinux/Makefile            |  2 +
> > > > > > > >  security/selinux/ima.c               | 64 ++++++++++++++++++++++++++++
> > > > > > > >  security/selinux/include/ima.h       | 24 +++++++++++
> > > > > > > >  security/selinux/include/security.h  |  3 +-
> > > > > > > >  security/selinux/ss/services.c       | 64 ++++++++++++++++++++++++----
> > > > > > > >  6 files changed, 149 insertions(+), 11 deletions(-)
> > > > > > > >  create mode 100644 security/selinux/ima.c
> > > > > > > >  create mode 100644 security/selinux/include/ima.h
> > > > > > >
> > > > > > > I remain concerned about the possibility of bypassing a measurement by
> > > > > > > tampering with the time, but I appear to be the only one who is
> > > > > > > worried about this so I'm not going to block this patch on those
> > > > > > > grounds.
> > > > > > >
> > > > > > > Acked-by: Paul Moore <[e-mail]>
> > > > > >
> > > > > > Thanks, Paul.
> > > > > >
> > > > > > Including any unique string would cause the buffer hash to change,
> > > > > > forcing a new measurement.  Perhaps they were concerned with
> > > > > > overflowing a counter.
> > > > >
> > > > > My understanding is that Lakshmi wanted to force a new measurement
> > > > > each time and felt using a timestamp would be the best way to do that.
> > > > > A counter, even if it wraps, would have a different value each time
> > > > > whereas a timestamp is vulnerable to time adjustments.  While a
> > > > > properly controlled and audited system could be configured and
> > > > > monitored to detect such an event (I *think*), why rely on that if it
> > > > > isn't necessary?
> > > >
> > > > Why are you saying that even if the counter wraps a new measurement is
> > > > guaranteed.   I agree with the rest of what you said.
> > >
> > > I was assuming that the IMA code simply compares the passed
> > > "policy_event_name" value to the previous value, if they are different
> > > a new measurement is taken, if they are the same the measurement
> > > request is ignored.  If this is the case the counter value is only
> > > important in as much as that it is different from the previous value,
> > > even simply toggling a single bit back and forth would suffice in this
> > > case.  IMA doesn't keep a record of every previous "policy_event_name"
> > > value does it?  Am I misunderstanding how
> > > ima_measure_critical_data(...) works?
> >
> > Originally, there was quite a bit of discussion as to how much or how
> > little should be measured for a number of reasons.  One reason is that
> > the TPM is relatively slow.  Another reason is to limit the size of the
> > measurement list.  For this reason, duplicate hashes aren't added to
> > the measurement list or extended into the TPM.
> >
> > When a dentry is removed from cache, its also removed from IMA's iint
> > cache.  A subsequent file read would result in adding the measurement
> > and extending the TPM again.  ima_lookup_digest_entry() is called to
> > prevent adding the duplicate entry.
> >
> > Lakshmi is trying to address the situation where an event changes a
> > value, but then is restored to the original value.  The original and
> > subsequent events are measured, but restoring to the original value
> > isn't re-measured.  This isn't any different than when a file is
> > modified and then reverted.
> >
> > Instead of changing the name like this, which doesn't work for files,
> > allowing duplicate measurements should be generic, based on policy.
> 
> Perhaps it is just the end of the day and I'm a bit tired, but I just
> read all of the above and I have no idea what your current thoughts
> are regarding this patch.

Other than appending the timestamp, which is a hack, the patch is fine.
Support for re-measuring an event can be upstreamed independently.

Mimi

Lakshmi Ramasubramanian Jan. 14, 2021, 4:22 p.m. UTC | #10

On 1/13/21 6:49 PM, Mimi Zohar wrote:

Hi Mimi,

>>>>>>>> I remain concerned about the possibility of bypassing a measurement by
>>>>>>>> tampering with the time, but I appear to be the only one who is
>>>>>>>> worried about this so I'm not going to block this patch on those
>>>>>>>> grounds.
>>>>>>>>
>>>>>>>> Acked-by: Paul Moore <[e-mail]>
>>>>>>>
>>>>>>> Thanks, Paul.
>>>>>>>
>>>>>>> Including any unique string would cause the buffer hash to change,
>>>>>>> forcing a new measurement.  Perhaps they were concerned with
>>>>>>> overflowing a counter.
>>>>>>
>>>>>> My understanding is that Lakshmi wanted to force a new measurement
>>>>>> each time and felt using a timestamp would be the best way to do that.
>>>>>> A counter, even if it wraps, would have a different value each time
>>>>>> whereas a timestamp is vulnerable to time adjustments.  While a
>>>>>> properly controlled and audited system could be configured and
>>>>>> monitored to detect such an event (I *think*), why rely on that if it
>>>>>> isn't necessary?
>>>>>
>>>>> Why are you saying that even if the counter wraps a new measurement is
>>>>> guaranteed.   I agree with the rest of what you said.
>>>>
>>>> I was assuming that the IMA code simply compares the passed
>>>> "policy_event_name" value to the previous value, if they are different
>>>> a new measurement is taken, if they are the same the measurement
>>>> request is ignored.  If this is the case the counter value is only
>>>> important in as much as that it is different from the previous value,
>>>> even simply toggling a single bit back and forth would suffice in this
>>>> case.  IMA doesn't keep a record of every previous "policy_event_name"
>>>> value does it?  Am I misunderstanding how
>>>> ima_measure_critical_data(...) works?
>>>
>>> Originally, there was quite a bit of discussion as to how much or how
>>> little should be measured for a number of reasons.  One reason is that
>>> the TPM is relatively slow.  Another reason is to limit the size of the
>>> measurement list.  For this reason, duplicate hashes aren't added to
>>> the measurement list or extended into the TPM.
>>>
>>> When a dentry is removed from cache, its also removed from IMA's iint
>>> cache.  A subsequent file read would result in adding the measurement
>>> and extending the TPM again.  ima_lookup_digest_entry() is called to
>>> prevent adding the duplicate entry.
>>>
>>> Lakshmi is trying to address the situation where an event changes a
>>> value, but then is restored to the original value.  The original and
>>> subsequent events are measured, but restoring to the original value
>>> isn't re-measured.  This isn't any different than when a file is
>>> modified and then reverted.
>>>
>>> Instead of changing the name like this, which doesn't work for files,
>>> allowing duplicate measurements should be generic, based on policy.
>>
>> Perhaps it is just the end of the day and I'm a bit tired, but I just
>> read all of the above and I have no idea what your current thoughts
>> are regarding this patch.
> 
> Other than appending the timestamp, which is a hack, the patch is fine.
> Support for re-measuring an event can be upstreamed independently.
> 

Thanks for clarifying the details related to duplicate measurement 
detection and re-measuring.

I will keep the timestamp for the time being, even though its a hack, as 
it helps with re-measuring state changes in SELinux. We will add support 
for "policy driven" re-measurement as a subsequent patch series.

thanks,
  -lakshmi

Mimi Zohar Jan. 14, 2021, 4:44 p.m. UTC | #11

[Cc'ing Sasha]

Hi Lakshmi,

On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote:
> On 1/13/21 6:49 PM, Mimi Zohar wrote:

> >>> Lakshmi is trying to address the situation where an event changes a
> >>> value, but then is restored to the original value.  The original and
> >>> subsequent events are measured, but restoring to the original value
> >>> isn't re-measured.  This isn't any different than when a file is
> >>> modified and then reverted.
> >>>
> >>> Instead of changing the name like this, which doesn't work for files,
> >>> allowing duplicate measurements should be generic, based on policy.
> >>
> >> Perhaps it is just the end of the day and I'm a bit tired, but I just
> >> read all of the above and I have no idea what your current thoughts
> >> are regarding this patch.
> > 
> > Other than appending the timestamp, which is a hack, the patch is fine.
> > Support for re-measuring an event can be upstreamed independently.
> > 
> 
> Thanks for clarifying the details related to duplicate measurement 
> detection and re-measuring.
> 
> I will keep the timestamp for the time being, even though its a hack, as 
> it helps with re-measuring state changes in SELinux. We will add support 
> for "policy driven" re-measurement as a subsequent patch series.

Once including the timestamp is upstreamed, removing it will be
difficult, especially if different userspace applications are dependent
on it.  Unless everyone is on board that removing the timestamp
wouldn't be considered a regression, it cannot be upstreamed.

thanks,

Mimi

Mimi Zohar Jan. 14, 2021, 4:50 p.m. UTC | #12

On Thu, 2021-01-14 at 11:44 -0500, Mimi Zohar wrote:
> [Cc'ing Sasha]
> 
> Hi Lakshmi,
> 
> On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote:
> > On 1/13/21 6:49 PM, Mimi Zohar wrote:
> 
> > >>> Lakshmi is trying to address the situation where an event changes a
> > >>> value, but then is restored to the original value.  The original and
> > >>> subsequent events are measured, but restoring to the original value
> > >>> isn't re-measured.  This isn't any different than when a file is
> > >>> modified and then reverted.
> > >>>
> > >>> Instead of changing the name like this, which doesn't work for files,
> > >>> allowing duplicate measurements should be generic, based on policy.
> > >>
> > >> Perhaps it is just the end of the day and I'm a bit tired, but I just
> > >> read all of the above and I have no idea what your current thoughts
> > >> are regarding this patch.
> > > 
> > > Other than appending the timestamp, which is a hack, the patch is fine.
> > > Support for re-measuring an event can be upstreamed independently.
> > > 
> > 
> > Thanks for clarifying the details related to duplicate measurement 
> > detection and re-measuring.
> > 
> > I will keep the timestamp for the time being, even though its a hack, as 
> > it helps with re-measuring state changes in SELinux. We will add support 
> > for "policy driven" re-measurement as a subsequent patch series.
> 
> Once including the timestamp is upstreamed, removing it will be
> difficult, especially if different userspace applications are dependent
> on it.  Unless everyone is on board that removing the timestamp
> wouldn't be considered a regression, it cannot be upstreamed.

Feel free to just re-post just this one patch.  Otherwise the patch set
looks good.

thanks,

Mimi

Paul Moore Jan. 14, 2021, 4:51 p.m. UTC | #13

On Thu, Jan 14, 2021 at 11:44 AM Mimi Zohar <[e-mail]> wrote:
>
> [Cc'ing Sasha]
>
> Hi Lakshmi,
>
> On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote:
> > On 1/13/21 6:49 PM, Mimi Zohar wrote:
>
> > >>> Lakshmi is trying to address the situation where an event changes a
> > >>> value, but then is restored to the original value.  The original and
> > >>> subsequent events are measured, but restoring to the original value
> > >>> isn't re-measured.  This isn't any different than when a file is
> > >>> modified and then reverted.
> > >>>
> > >>> Instead of changing the name like this, which doesn't work for files,
> > >>> allowing duplicate measurements should be generic, based on policy.
> > >>
> > >> Perhaps it is just the end of the day and I'm a bit tired, but I just
> > >> read all of the above and I have no idea what your current thoughts
> > >> are regarding this patch.
> > >
> > > Other than appending the timestamp, which is a hack, the patch is fine.
> > > Support for re-measuring an event can be upstreamed independently.
> > >
> >
> > Thanks for clarifying the details related to duplicate measurement
> > detection and re-measuring.
> >
> > I will keep the timestamp for the time being, even though its a hack, as
> > it helps with re-measuring state changes in SELinux. We will add support
> > for "policy driven" re-measurement as a subsequent patch series.
>
> Once including the timestamp is upstreamed, removing it will be
> difficult, especially if different userspace applications are dependent
> on it.  Unless everyone is on board that removing the timestamp
> wouldn't be considered a regression, it cannot be upstreamed.

I'm not a fan of merging things which are known to be broken only with
the promise of fixing it later.  That goes double when the proper fix
will result in a user visible breaking change.

Lakshmi Ramasubramanian Jan. 14, 2021, 5:48 p.m. UTC | #14

On 1/14/21 8:50 AM, Mimi Zohar wrote:
> On Thu, 2021-01-14 at 11:44 -0500, Mimi Zohar wrote:
>> [Cc'ing Sasha]
>>
>> Hi Lakshmi,
>>
>> On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote:
>>> On 1/13/21 6:49 PM, Mimi Zohar wrote:
>>
>>>>>> Lakshmi is trying to address the situation where an event changes a
>>>>>> value, but then is restored to the original value.  The original and
>>>>>> subsequent events are measured, but restoring to the original value
>>>>>> isn't re-measured.  This isn't any different than when a file is
>>>>>> modified and then reverted.
>>>>>>
>>>>>> Instead of changing the name like this, which doesn't work for files,
>>>>>> allowing duplicate measurements should be generic, based on policy.
>>>>>
>>>>> Perhaps it is just the end of the day and I'm a bit tired, but I just
>>>>> read all of the above and I have no idea what your current thoughts
>>>>> are regarding this patch.
>>>>
>>>> Other than appending the timestamp, which is a hack, the patch is fine.
>>>> Support for re-measuring an event can be upstreamed independently.
>>>>
>>>
>>> Thanks for clarifying the details related to duplicate measurement
>>> detection and re-measuring.
>>>
>>> I will keep the timestamp for the time being, even though its a hack, as
>>> it helps with re-measuring state changes in SELinux. We will add support
>>> for "policy driven" re-measurement as a subsequent patch series.
>>
>> Once including the timestamp is upstreamed, removing it will be
>> difficult, especially if different userspace applications are dependent
>> on it.  Unless everyone is on board that removing the timestamp
>> wouldn't be considered a regression, it cannot be upstreamed.
> 
> Feel free to just re-post just this one patch.  Otherwise the patch set
> looks good.
> 
> thanks,
> 

Sounds good Mimi - I will remove the timestamp and re-post the selinux 
patch.

thanks,
  -lakshmi

Lakshmi Ramasubramanian Jan. 14, 2021, 7:21 p.m. UTC | #15

On 1/14/21 9:48 AM, Lakshmi Ramasubramanian wrote:
> On 1/14/21 8:50 AM, Mimi Zohar wrote:
>> On Thu, 2021-01-14 at 11:44 -0500, Mimi Zohar wrote:
>>> [Cc'ing Sasha]
>>>
>>> Hi Lakshmi,
>>>
>>> On Thu, 2021-01-14 at 08:22 -0800, Lakshmi Ramasubramanian wrote:
>>>> On 1/13/21 6:49 PM, Mimi Zohar wrote:
>>>
>>>>>>> Lakshmi is trying to address the situation where an event changes a
>>>>>>> value, but then is restored to the original value.  The original and
>>>>>>> subsequent events are measured, but restoring to the original value
>>>>>>> isn't re-measured.  This isn't any different than when a file is
>>>>>>> modified and then reverted.
>>>>>>>
>>>>>>> Instead of changing the name like this, which doesn't work for 
>>>>>>> files,
>>>>>>> allowing duplicate measurements should be generic, based on policy.
>>>>>>
>>>>>> Perhaps it is just the end of the day and I'm a bit tired, but I just
>>>>>> read all of the above and I have no idea what your current thoughts
>>>>>> are regarding this patch.
>>>>>
>>>>> Other than appending the timestamp, which is a hack, the patch is 
>>>>> fine.
>>>>> Support for re-measuring an event can be upstreamed independently.
>>>>>
>>>>
>>>> Thanks for clarifying the details related to duplicate measurement
>>>> detection and re-measuring.
>>>>
>>>> I will keep the timestamp for the time being, even though its a 
>>>> hack, as
>>>> it helps with re-measuring state changes in SELinux. We will add 
>>>> support
>>>> for "policy driven" re-measurement as a subsequent patch series.
>>>
>>> Once including the timestamp is upstreamed, removing it will be
>>> difficult, especially if different userspace applications are dependent
>>> on it.  Unless everyone is on board that removing the timestamp
>>> wouldn't be considered a regression, it cannot be upstreamed.
>>
>> Feel free to just re-post just this one patch.  Otherwise the patch set
>> looks good.
>>
>> thanks,
>>
> 
> Sounds good Mimi - I will remove the timestamp and re-post the selinux 
> patch.
> 

I have removed the timestamp in the event name and have posted the 
selinux patch alone.

Thanks a lot for reviewing the changes.

  -lakshmi

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 54fe1c15ed50..8365596cb42b 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -52,8 +52,9 @@  Description:
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value
-			label:= [data_label]
+			label:= [selinux]|[data_label]
 			data_label:= a unique string used for grouping and limiting critical data.
+			For example, "selinux" to measure critical data for SELinux.
 
 		  default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/selinux/Makefile b/security/selinux/Makefile
index 4d8e0e8adf0b..776162444882 100644
--- a/security/selinux/Makefile
+++ b/security/selinux/Makefile
@@ -16,6 +16,8 @@  selinux-$(CONFIG_NETLABEL) += netlabel.o
 
 selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o
 
+selinux-$(CONFIG_IMA) += ima.o
+
 ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include
 
 $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h
diff --git a/security/selinux/ima.c b/security/selinux/ima.c
new file mode 100644
index 000000000000..0b835bdc3aa9
--- /dev/null
+++ b/security/selinux/ima.c
@@ -0,0 +1,64 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2021 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian ([e-mail])
+ *
+ * Measure critical data structures maintainted by SELinux
+ * using IMA subsystem.
+ */
+#include <linux/vmalloc.h>
+#include <linux/ktime.h>
+#include <linux/ima.h>
+#include "security.h"
+#include "ima.h"
+
+/*
+ * selinux_ima_measure_state - Measure hash of the SELinux policy
+ *
+ * @state: selinux state struct
+ *
+ * NOTE: This function must be called with policy_mutex held.
+ */
+void selinux_ima_measure_state(struct selinux_state *state)
+{
+	struct timespec64 cur_time;
+	void *policy = NULL;
+	char *policy_event_name = NULL;
+	size_t policy_len;
+	int rc = 0;
+
+	/*
+	 * Measure SELinux policy only after initialization is completed.
+	 */
+	if (!selinux_initialized(state))
+		return;
+
+	/*
+	 * Pass a unique "event_name" to the IMA hook so that IMA subsystem
+	 * will always measure the given data.
+	 */
+	ktime_get_real_ts64(&cur_time);
+	policy_event_name = kasprintf(GFP_KERNEL, "%s-%lld:%09ld",
+				      "selinux-policy-hash",
+				      cur_time.tv_sec, cur_time.tv_nsec);
+	if (!policy_event_name) {
+		pr_err("SELinux: %s: event name for policy not allocated.\n",
+		       __func__);
+		goto out;
+	}
+
+	rc = security_read_state_kernel(state, &policy, &policy_len);
+	if (rc) {
+		pr_err("SELinux: %s: failed to read policy %d.\n", __func__, rc);
+		goto out;
+	}
+
+	ima_measure_critical_data("selinux", policy_event_name,
+				  policy, policy_len, true);
+
+	vfree(policy);
+
+out:
+	kfree(policy_event_name);
+}
diff --git a/security/selinux/include/ima.h b/security/selinux/include/ima.h
new file mode 100644
index 000000000000..d69c36611423
--- /dev/null
+++ b/security/selinux/include/ima.h
@@ -0,0 +1,24 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2021 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian ([e-mail])
+ *
+ * Measure critical data structures maintainted by SELinux
+ * using IMA subsystem.
+ */
+
+#ifndef _SELINUX_IMA_H_
+#define _SELINUX_IMA_H_
+
+#include "security.h"
+
+#ifdef CONFIG_IMA
+extern void selinux_ima_measure_state(struct selinux_state *selinux_state);
+#else
+static inline void selinux_ima_measure_state(struct selinux_state *selinux_state)
+{
+}
+#endif
+
+#endif	/* _SELINUX_IMA_H_ */
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 3cc8bab31ea8..29cae32d3fc5 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -229,7 +229,8 @@  void selinux_policy_cancel(struct selinux_state *state,
 			struct selinux_policy *policy);
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
-
+int security_read_state_kernel(struct selinux_state *state,
+			       void **data, size_t *len);
 int security_policycap_supported(struct selinux_state *state,
 				 unsigned int req_cap);
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 9704c8a32303..cc8dbc4ed8db 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -65,6 +65,7 @@ 
 #include "ebitmap.h"
 #include "audit.h"
 #include "policycap_names.h"
+#include "ima.h"
 
 /* Forward declaration. */
 static int context_struct_to_string(struct policydb *policydb,
@@ -2180,6 +2181,7 @@  static void selinux_notify_policy_change(struct selinux_state *state,
 	selinux_status_update_policyload(state, seqno);
 	selinux_netlbl_cache_invalidate();
 	selinux_xfrm_notify_policyload();
+	selinux_ima_measure_state(state);
 }
 
 void selinux_policy_commit(struct selinux_state *state,
@@ -3875,8 +3877,33 @@  int security_netlbl_sid_to_secattr(struct selinux_state *state,
 }
 #endif /* CONFIG_NETLABEL */
 
+/**
+ * __security_read_policy - read the policy.
+ * @policy: SELinux policy
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ */
+static int __security_read_policy(struct selinux_policy *policy,
+				  void *data, size_t *len)
+{
+	int rc;
+	struct policy_file fp;
+
+	fp.data = data;
+	fp.len = *len;
+
+	rc = policydb_write(&policy->policydb, &fp);
+	if (rc)
+		return rc;
+
+	*len = (unsigned long)fp.data - (unsigned long)data;
+	return 0;
+}
+
 /**
  * security_read_policy - read the policy.
+ * @state: selinux_state
  * @data: binary policy data
  * @len: length of data in bytes
  *
@@ -3885,8 +3912,6 @@  int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len)
 {
 	struct selinux_policy *policy;
-	int rc;
-	struct policy_file fp;
 
 	policy = rcu_dereference_protected(
 			state->policy, lockdep_is_held(&state->policy_mutex));
@@ -3898,14 +3923,35 @@  int security_read_policy(struct selinux_state *state,
 	if (!*data)
 		return -ENOMEM;
 
-	fp.data = *data;
-	fp.len = *len;
+	return __security_read_policy(policy, *data, len);
+}
 
-	rc = policydb_write(&policy->policydb, &fp);
-	if (rc)
-		return rc;
+/**
+ * security_read_state_kernel - read the policy.
+ * @state: selinux_state
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ * Allocates kernel memory for reading SELinux policy.
+ * This function is for internal use only and should not
+ * be used for returning data to user space.
+ *
+ * This function must be called with policy_mutex held.
+ */
+int security_read_state_kernel(struct selinux_state *state,
+			       void **data, size_t *len)
+{
+	struct selinux_policy *policy;
 
-	*len = (unsigned long)fp.data - (unsigned long)*data;
-	return 0;
+	policy = rcu_dereference_protected(
+			state->policy, lockdep_is_held(&state->policy_mutex));
+	if (!policy)
+		return -EINVAL;
+
+	*len = policy->policydb.len;
+	*data = vmalloc(*len);
+	if (!*data)
+		return -ENOMEM;
 
+	return __security_read_policy(policy, *data, len);
 }