diff mbox series

[v28,22/25] Audit: Add record for multiple process LSM attributes

Message ID 20210722004758.12371-23-casey@schaufler-ca.com (mailing list archive)
State Superseded
Headers show
Series [v28,01/25] LSM: Infrastructure management of the sock security | expand

Commit Message

Casey Schaufler July 22, 2021, 12:47 a.m. UTC
Create a new audit record type to contain the subject information
when there are multiple security modules that require such data.
This record is linked with the same timestamp and serial number
using the audit_alloc_local() mechanism.
The record is produced only in cases where there is more than one
security module with a process "context".
In cases where this record is produced the subj= fields of
other records in the audit event will be set to "subj=?".

An example of the MAC_TASK_CONTEXTS (1420) record is:

        type=UNKNOWN[1420]
        msg=audit(1600880931.832:113)
        subj_apparmor="=unconfined"
        subj_smack="_"

There will be a subj_$LSM= entry for each security module
LSM that supports the secid_to_secctx and secctx_to_secid
hooks. The BPF security module implements secid/secctx
translation hooks, so it has to be considered to provide a
secctx even though it may not actually do so.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
To: paul@paul-moore.com
To: linux-audit@redhat.com
To: rgb@redhat.com
Cc: netdev@vger.kernel.org
---
 drivers/android/binder.c                |  2 +-
 include/linux/audit.h                   | 16 +++++
 include/linux/security.h                | 16 ++++-
 include/net/netlabel.h                  |  2 +-
 include/net/scm.h                       |  2 +-
 include/net/xfrm.h                      | 13 +++-
 include/uapi/linux/audit.h              |  1 +
 kernel/audit.c                          | 90 +++++++++++++++++++------
 kernel/auditfilter.c                    |  5 +-
 kernel/auditsc.c                        | 27 ++++++--
 net/ipv4/ip_sockglue.c                  |  2 +-
 net/netfilter/nf_conntrack_netlink.c    |  4 +-
 net/netfilter/nf_conntrack_standalone.c |  2 +-
 net/netfilter/nfnetlink_queue.c         |  2 +-
 net/netlabel/netlabel_unlabeled.c       | 21 +++---
 net/netlabel/netlabel_user.c            | 14 ++--
 net/netlabel/netlabel_user.h            |  6 +-
 net/xfrm/xfrm_policy.c                  |  8 ++-
 net/xfrm/xfrm_state.c                   | 18 +++--
 security/integrity/ima/ima_api.c        |  6 +-
 security/integrity/integrity_audit.c    |  5 +-
 security/security.c                     | 46 ++++++++-----
 security/smack/smackfs.c                |  3 +-
 23 files changed, 221 insertions(+), 90 deletions(-)

Comments

kernel test robot July 22, 2021, 4:08 a.m. UTC | #1
Hi Casey,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pcmoore-audit/next]
[also build test WARNING on nf/master linus/master v5.14-rc2]
[cannot apply to nf-next/master security/next-testing next-20210721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Casey-Schaufler/LSM-Infrastructure-management-of-the-sock-security/20210722-094735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/06ec035c23ade985d4661768f52f2e8cda1ce313
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Casey-Schaufler/LSM-Infrastructure-management-of-the-sock-security/20210722-094735
        git checkout 06ec035c23ade985d4661768f52f2e8cda1ce313
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from kernel/audit.c:51:
   include/linux/audit.h:571:1: error: expected identifier or '(' before '+' token
     571 | +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
         | ^
   kernel/audit.c: In function 'audit_log_config_change':
   kernel/audit.c:393:12: error: implicit declaration of function 'audit_alloc_for_lsm'; did you mean 'audit_alloc_mark'? [-Werror=implicit-function-declaration]
     393 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^~~~~~~~~~~~~~~~~~~
         |            audit_alloc_mark
>> kernel/audit.c:393:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     393 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   kernel/audit.c: In function 'audit_receive_msg':
   kernel/audit.c:1358:13: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1358 |    lcontext = audit_alloc_for_lsm(GFP_KERNEL);
         |             ^
   kernel/audit.c:1381:13: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1381 |    lcontext = audit_alloc_for_lsm(GFP_KERNEL);
         |             ^
   kernel/audit.c:1399:12: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1399 |   lcontext = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^
   kernel/audit.c:1431:12: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1431 |   lcontext = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^
   kernel/audit.c:1505:12: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1505 |   lcontext = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^
   kernel/audit.c: In function 'audit_log_multicast':
   kernel/audit.c:1567:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1567 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   kernel/audit.c: At top level:
   kernel/audit.c:1789:14: warning: no previous prototype for 'audit_serial' [-Wmissing-prototypes]
    1789 | unsigned int audit_serial(void)
         |              ^~~~~~~~~~~~
   kernel/audit.c: In function 'audit_log_vformat':
   kernel/audit.c:1937:2: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1937 |  len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args);
         |  ^~~
   kernel/audit.c:1946:3: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1946 |   len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args2);
         |   ^~~
   kernel/audit.c: In function 'audit_log_set_loginuid':
   kernel/audit.c:2334:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    2334 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   In file included from include/linux/perf_event.h:25,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:87,
                    from kernel/audit.c:44:
   At top level:
   arch/arc/include/asm/perf_event.h:126:23: warning: 'arc_pmu_cache_map' defined but not used [-Wunused-const-variable=]
     126 | static const unsigned arc_pmu_cache_map[C(MAX)][C(OP_MAX)][C(RESULT_MAX)] = {
         |                       ^~~~~~~~~~~~~~~~~
   arch/arc/include/asm/perf_event.h:91:27: warning: 'arc_pmu_ev_hw_map' defined but not used [-Wunused-const-variable=]
      91 | static const char * const arc_pmu_ev_hw_map[] = {
         |                           ^~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from kernel/auditfilter.c:12:
   include/linux/audit.h:571:1: error: expected identifier or '(' before '+' token
     571 | +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
         | ^
   kernel/auditfilter.c: In function 'audit_log_rule_change':
   kernel/auditfilter.c:1107:12: error: implicit declaration of function 'audit_alloc_for_lsm'; did you mean 'audit_alloc_mark'? [-Werror=implicit-function-declaration]
    1107 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^~~~~~~~~~~~~~~~~~~
         |            audit_alloc_mark
>> kernel/auditfilter.c:1107:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1107 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   cc1: some warnings being treated as errors
--
   In file included from security/integrity/integrity_audit.c:12:
   include/linux/audit.h:571:1: error: expected identifier or '(' before '+' token
     571 | +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
         | ^
   security/integrity/integrity_audit.c: In function 'integrity_audit_message':
   security/integrity/integrity_audit.c:48:12: error: implicit declaration of function 'audit_alloc_for_lsm'; did you mean 'audit_log_format'? [-Werror=implicit-function-declaration]
      48 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^~~~~~~~~~~~~~~~~~~
         |            audit_log_format
>> security/integrity/integrity_audit.c:48:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      48 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   cc1: some warnings being treated as errors
--
   In file included from security/integrity/ima/ima.h:22,
                    from security/integrity/ima/ima_api.c:18:
   include/linux/audit.h:571:1: error: expected identifier or '(' before '+' token
     571 | +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
         | ^
   security/integrity/ima/ima_api.c: In function 'ima_audit_measurement':
   security/integrity/ima/ima_api.c:362:12: error: implicit declaration of function 'audit_alloc_for_lsm'; did you mean 'audit_log_format'? [-Werror=implicit-function-declaration]
     362 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |            ^~~~~~~~~~~~~~~~~~~
         |            audit_log_format
>> security/integrity/ima/ima_api.c:362:10: warning: assignment to 'struct audit_context *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     362 |  context = audit_alloc_for_lsm(GFP_KERNEL);
         |          ^
   cc1: some warnings being treated as errors


vim +393 kernel/audit.c

   385	
   386	static int audit_log_config_change(char *function_name, u32 new, u32 old,
   387					   int allow_changes)
   388	{
   389		struct audit_context *context;
   390		struct audit_buffer *ab;
   391		int rc = 0;
   392	
 > 393		context = audit_alloc_for_lsm(GFP_KERNEL);
   394		ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
   395		if (unlikely(!ab))
   396			return rc;
   397		audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
   398		audit_log_session_info(ab);
   399		rc = audit_log_task_context(ab);
   400		if (rc)
   401			allow_changes = 0; /* Something weird, deny request */
   402		audit_log_format(ab, " res=%d", allow_changes);
   403		audit_log_end(ab);
   404		audit_free_local(context);
   405		return rc;
   406	}
   407	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 12, 2021, 8:59 p.m. UTC | #2
On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Create a new audit record type to contain the subject information
> when there are multiple security modules that require such data.
> This record is linked with the same timestamp and serial number
> using the audit_alloc_local() mechanism.
> The record is produced only in cases where there is more than one
> security module with a process "context".
> In cases where this record is produced the subj= fields of
> other records in the audit event will be set to "subj=?".
>
> An example of the MAC_TASK_CONTEXTS (1420) record is:
>
>         type=UNKNOWN[1420]
>         msg=audit(1600880931.832:113)
>         subj_apparmor="=unconfined"
>         subj_smack="_"
>
> There will be a subj_$LSM= entry for each security module
> LSM that supports the secid_to_secctx and secctx_to_secid
> hooks. The BPF security module implements secid/secctx
> translation hooks, so it has to be considered to provide a
> secctx even though it may not actually do so.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> To: paul@paul-moore.com
> To: linux-audit@redhat.com
> To: rgb@redhat.com
> Cc: netdev@vger.kernel.org
> ---
>  drivers/android/binder.c                |  2 +-
>  include/linux/audit.h                   | 16 +++++
>  include/linux/security.h                | 16 ++++-
>  include/net/netlabel.h                  |  2 +-
>  include/net/scm.h                       |  2 +-
>  include/net/xfrm.h                      | 13 +++-
>  include/uapi/linux/audit.h              |  1 +
>  kernel/audit.c                          | 90 +++++++++++++++++++------
>  kernel/auditfilter.c                    |  5 +-
>  kernel/auditsc.c                        | 27 ++++++--
>  net/ipv4/ip_sockglue.c                  |  2 +-
>  net/netfilter/nf_conntrack_netlink.c    |  4 +-
>  net/netfilter/nf_conntrack_standalone.c |  2 +-
>  net/netfilter/nfnetlink_queue.c         |  2 +-
>  net/netlabel/netlabel_unlabeled.c       | 21 +++---
>  net/netlabel/netlabel_user.c            | 14 ++--
>  net/netlabel/netlabel_user.h            |  6 +-
>  net/xfrm/xfrm_policy.c                  |  8 ++-
>  net/xfrm/xfrm_state.c                   | 18 +++--
>  security/integrity/ima/ima_api.c        |  6 +-
>  security/integrity/integrity_audit.c    |  5 +-
>  security/security.c                     | 46 ++++++++-----
>  security/smack/smackfs.c                |  3 +-
>  23 files changed, 221 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 2c3a2348a144..3520caa0260c 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2722,7 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>                  * case well anyway.
>                  */
>                 security_task_getsecid_obj(proc->tsk, &blob);
> -               ret = security_secid_to_secctx(&blob, &lsmctx);
> +               ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
>                 if (ret) {
>                         return_error = BR_FAILED_REPLY;
>                         return_error_param = ret;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 97cd7471e572..85eb87f6f92d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -291,6 +291,7 @@ extern int  audit_alloc(struct task_struct *task);
>  extern void __audit_free(struct task_struct *task);
>  extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
>  extern void audit_free_context(struct audit_context *context);
> +extern void audit_free_local(struct audit_context *context);
>  extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>                                   unsigned long a2, unsigned long a3);
>  extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -386,6 +387,19 @@ static inline void audit_ptrace(struct task_struct *t)
>                 __audit_ptrace(t);
>  }
>
> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
> +{
> +       struct audit_context *context = audit_context();
> +
> +       if (context)
> +               return context;
> +
> +       if (lsm_multiple_contexts())
> +               return audit_alloc_local(gfp);
> +
> +       return NULL;
> +}

We don't want to do this, at least not as it is written above.  We
shouldn't have a function which abstracts away the creation of a local
audit_context.  Usage of a local audit_context should be explicit in
the caller, and if the caller's execution context is ambiguous enough
that it can require both a task_struct audit_context and a local
audit_context then we need to handle that on a case-by-case basis.
Hiding it like this is guaranteed to cause problems later.

I probably did a poor job of explaining what a local context is during
the last patchset; I'll try to do a better job here but also let me
say this as clear as I can ... if the "current" task struct is valid
for a given code path, *never* use a local audit context.  The local
audit context is a hack that is made necessary by the fact that we
have to audit things which happen outside the scope of an executing
task, e.g. the netfilter audit hooks, it should *never* be used when
there is a valid task_struct.

It's the audit_context which helps bind multiple audit records into a
single event, creating a new, "local" audit_context destroys that
binding as audit records created using that local audit_context have a
different timestamp from those records created using the current
task_struct's audit_context.

Hopefully that makes sense?

>                                 /* Private API (for audit.c only) */
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
> @@ -560,6 +574,8 @@ extern int audit_signals;
>  }
>  static inline void audit_free_context(struct audit_context *context)
>  { }
> +static inline void audit_free_local(struct audit_context *context)
> +{ }
>  static inline int audit_alloc(struct task_struct *task)
>  {
>         return 0;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3e9743118fb9..b3cf68cf2bd6 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -182,6 +182,8 @@ struct lsmblob {
>  #define LSMBLOB_INVALID                -1      /* Not a valid LSM slot number */
>  #define LSMBLOB_NEEDED         -2      /* Slot requested on initialization */
>  #define LSMBLOB_NOT_NEEDED     -3      /* Slot not requested */
> +#define LSMBLOB_DISPLAY                -4      /* Use the "display" slot */
> +#define LSMBLOB_FIRST          -5      /* Use the default "display" slot */
>
>  /**
>   * lsmblob_init - initialize an lsmblob structure
> @@ -248,6 +250,15 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
>         return 0;
>  }
>
> +static inline bool lsm_multiple_contexts(void)
> +{
> +#ifdef CONFIG_SECURITY
> +       return lsm_slot_to_name(1) != NULL;
> +#else
> +       return false;
> +#endif
> +}
> +
>  /* These functions are in security/commoncap.c */
>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>                        int cap, unsigned int opts);
> @@ -578,7 +589,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>                          size_t size);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> +                            int display);
>  int security_secctx_to_secid(const char *secdata, u32 seclen,
>                              struct lsmblob *blob);
>  void security_release_secctx(struct lsmcontext *cp);
> @@ -1433,7 +1445,7 @@ static inline int security_ismaclabel(const char *name)
>  }
>
>  static inline int security_secid_to_secctx(struct lsmblob *blob,
> -                                          struct lsmcontext *cp)
> +                                          struct lsmcontext *cp, int display)
>  {
>         return -EOPNOTSUPP;
>  }
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 73fc25b4042b..216cb1ffc8f0 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -97,7 +97,7 @@ struct calipso_doi;
>
>  /* NetLabel audit information */
>  struct netlbl_audit {
> -       u32 secid;
> +       struct lsmblob lsmdata;
>         kuid_t loginuid;
>         unsigned int sessionid;
>  };

This chunk seems lost here, does it belong in another patch?

> diff --git a/include/net/scm.h b/include/net/scm.h
> index b77a52f93389..f4d567d4885e 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>                  * and the infrastructure will know which it is.
>                  */
>                 lsmblob_init(&lb, scm->secid);
> -               err = security_secid_to_secctx(&lb, &context);
> +               err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);

Misplaced code change?

>                 if (!err) {
>                         put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index cbff7c2a9724..a10fa01f7bf4 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -660,13 +660,22 @@ struct xfrm_spi_skb_cb {
>  #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
>
>  #ifdef CONFIG_AUDITSYSCALL
> -static inline struct audit_buffer *xfrm_audit_start(const char *op)
> +static inline struct audit_buffer *xfrm_audit_start(const char *op,
> +                                                   struct audit_context **lac)
>  {
> +       struct audit_context *context;
>         struct audit_buffer *audit_buf = NULL;
>
>         if (audit_enabled == AUDIT_OFF)
>                 return NULL;
> -       audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
> +       context = audit_context();
> +       if (lac != NULL) {
> +               if (lsm_multiple_contexts() && context == NULL)
> +                       context = audit_alloc_local(GFP_ATOMIC);
> +               *lac = context;
> +       }
> +
> +       audit_buf = audit_log_start(context, GFP_ATOMIC,
>                                     AUDIT_MAC_IPSEC_EVENT);
>         if (audit_buf == NULL)
>                 return NULL;

Related to the other comments around local audit_contexts, we don't
want to do this; use the existing audit_context, @lac in this case, so
that this audit record is bound to the other associated records into a
single audit event (all have the same timestamp).

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 841123390d41..cba63789a164 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -386,10 +386,12 @@ void audit_log_lost(const char *message)
>  static int audit_log_config_change(char *function_name, u32 new, u32 old,
>                                    int allow_changes)
>  {
> +       struct audit_context *context;
>         struct audit_buffer *ab;
>         int rc = 0;
>
> -       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +       context = audit_alloc_for_lsm(GFP_KERNEL);
> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);

We shouldn't need to do this for the reasons discussed up near the top
of this email (and elsewhere as well).

I'm going to refrain from commenting on the other uses of
audit_alloc_for_lsm() in this patch unless there is something unique
to the code path, so if you don't see me comment about a use of
audit_alloc_for_lsm() you can assume it should be removed and the
existing audit_context used instead.

> @@ -399,6 +401,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>                 allow_changes = 0; /* Something weird, deny request */
>         audit_log_format(ab, " res=%d", allow_changes);
>         audit_log_end(ab);
> +       audit_free_local(context);

See my comment directly above regarding usage of
audit_alloc_for_lsm(), it obviously applies here too.

> @@ -2128,6 +2136,36 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>                 audit_log_format(ab, "(null)");
>  }
>
> +static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)

See my note below about moving this into audit_log_task_context(), but
if we really need to keep this as a separate function, let's consider
changing the name to something which indicates that it logs the LSM
data as *subject* fields.  How about audit_log_lsm_subj()?

> +{
> +       struct audit_buffer *ab;
> +       struct lsmcontext lsmdata;
> +       bool sep = false;
> +       int error;
> +       int i;
> +
> +       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
> +       if (!ab)
> +               return; /* audit_panic or being filtered */
> +
> +       for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> +               if (blob->secid[i] == 0)
> +                       continue;
> +               error = security_secid_to_secctx(blob, &lsmdata, i);
> +               if (error && error != -EINVAL) {
> +                       audit_panic("error in audit_log_lsm");
> +                       return;
> +               }
> +
> +               audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
> +                                lsm_slot_to_name(i), lsmdata.context);
> +               sep = true;

Since @i starts at zero, you can get rid of @sep by replacing it with @i:

  audit_log_format(ab, ..., (i ? " " : ""), ...);

> +               security_release_secctx(&lsmdata);
> +       }
> +       audit_log_end(ab);
> +}

> @@ -2138,7 +2176,18 @@ int audit_log_task_context(struct audit_buffer *ab)
>         if (!lsmblob_is_set(&blob))
>                 return 0;
>
> -       error = security_secid_to_secctx(&blob, &context);
> +       /*
> +        * If there is more than one security module that has a
> +        * subject "context" it's necessary to put the subject data
> +        * into a separate record to maintain compatibility.
> +        */
> +       if (lsm_multiple_contexts()) {
> +               audit_log_format(ab, " subj=?");
> +               audit_log_lsm(ab->ctx, &blob);
> +               return 0;
> +       }
> +
> +       error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);

Instead of the lsm_multiple_contexts() case bailing on the rest of the
function with a return inside the if-block, let's made the code a bit
more robust by organizing it like this:

  int audit_log_task_context(ab)
  {
     /* common stuff at the start */

     if (lsm_multiple_contexts()) {
       /* multi-LSM stuff */
     } else {
       /* single LSM stuff */
     }

     /* common stuff at the end */
  }

... it also may make sense to just move the body of audit_log_lsm()
into audit_log_task_context() and do away with audit_log_lsm()
entirely; is it ever going to be called from anywhere else?

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0e58a3ab56f5..01fdcbf468c0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -993,12 +993,11 @@ struct audit_context *audit_alloc_local(gfp_t gfpflags)
>         context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
>         if (!context) {
>                 audit_log_lost("out of memory in audit_alloc_local");
> -               goto out;
> +               return NULL;
>         }
>         context->serial = audit_serial();
>         ktime_get_coarse_real_ts64(&context->ctime);
>         context->local = true;
> -out:
>         return context;
>  }

This chunk should be moved to 21/25 when audit_alloc_local() is first defined.

> @@ -1019,6 +1018,13 @@ void audit_free_context(struct audit_context *context)
>  }
>  EXPORT_SYMBOL(audit_free_context);
>
> +void audit_free_local(struct audit_context *context)
> +{
> +       if (context && context->local)
> +               audit_free_context(context);
> +}
> +EXPORT_SYMBOL(audit_free_local);

If this is strictly necessary, and I don't think it is, it should also
be moved to patch 21/25 with the original definition of a local
audit_context.  However,  there really should be no reason why we have
to distinguish between a proper and local audtit_context when it comes
to free'ing the memory, just call audit_free_context() in both cases.

> @@ -1036,7 +1042,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>                          from_kuid(&init_user_ns, auid),
>                          from_kuid(&init_user_ns, uid), sessionid);
>         if (lsmblob_is_set(blob)) {
> -               if (security_secid_to_secctx(blob, &lsmctx)) {
> +               if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {

Misplaced code change?

Actually, there are a lot of these below, I'm not going to comment on
all of them as I think you get the idea ... and I very well may be
wrong so I'll save you all of my wrongness in that case :)

> diff --git a/security/security.c b/security/security.c
> index cb359e185d1a..5d7fd982f84a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2309,7 +2309,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>                 hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>                                      list) {
>                         rc = hp->hook.setprocattr(name, value, size);
> -                       if (rc < 0)
> +                       if (rc < 0 && rc != -EINVAL)
>                                 return rc;
>                 }

This really looks misplaced ... ?
Casey Schaufler Aug. 12, 2021, 10:38 p.m. UTC | #3
On 8/12/2021 1:59 PM, Paul Moore wrote:
> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Create a new audit record type to contain the subject information
>> when there are multiple security modules that require such data.
>> This record is linked with the same timestamp and serial number
>> using the audit_alloc_local() mechanism.
>> The record is produced only in cases where there is more than one
>> security module with a process "context".
>> In cases where this record is produced the subj= fields of
>> other records in the audit event will be set to "subj=?".
>>
>> An example of the MAC_TASK_CONTEXTS (1420) record is:
>>
>>         type=UNKNOWN[1420]
>>         msg=audit(1600880931.832:113)
>>         subj_apparmor="=unconfined"
>>         subj_smack="_"
>>
>> There will be a subj_$LSM= entry for each security module
>> LSM that supports the secid_to_secctx and secctx_to_secid
>> hooks. The BPF security module implements secid/secctx
>> translation hooks, so it has to be considered to provide a
>> secctx even though it may not actually do so.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> To: paul@paul-moore.com
>> To: linux-audit@redhat.com
>> To: rgb@redhat.com
>> Cc: netdev@vger.kernel.org
>> ---
>>  drivers/android/binder.c                |  2 +-
>>  include/linux/audit.h                   | 16 +++++
>>  include/linux/security.h                | 16 ++++-
>>  include/net/netlabel.h                  |  2 +-
>>  include/net/scm.h                       |  2 +-
>>  include/net/xfrm.h                      | 13 +++-
>>  include/uapi/linux/audit.h              |  1 +
>>  kernel/audit.c                          | 90 +++++++++++++++++++------
>>  kernel/auditfilter.c                    |  5 +-
>>  kernel/auditsc.c                        | 27 ++++++--
>>  net/ipv4/ip_sockglue.c                  |  2 +-
>>  net/netfilter/nf_conntrack_netlink.c    |  4 +-
>>  net/netfilter/nf_conntrack_standalone.c |  2 +-
>>  net/netfilter/nfnetlink_queue.c         |  2 +-
>>  net/netlabel/netlabel_unlabeled.c       | 21 +++---
>>  net/netlabel/netlabel_user.c            | 14 ++--
>>  net/netlabel/netlabel_user.h            |  6 +-
>>  net/xfrm/xfrm_policy.c                  |  8 ++-
>>  net/xfrm/xfrm_state.c                   | 18 +++--
>>  security/integrity/ima/ima_api.c        |  6 +-
>>  security/integrity/integrity_audit.c    |  5 +-
>>  security/security.c                     | 46 ++++++++-----
>>  security/smack/smackfs.c                |  3 +-
>>  23 files changed, 221 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 2c3a2348a144..3520caa0260c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2722,7 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>>                  * case well anyway.
>>                  */
>>                 security_task_getsecid_obj(proc->tsk, &blob);
>> -               ret = security_secid_to_secctx(&blob, &lsmctx);
>> +               ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
>>                 if (ret) {
>>                         return_error = BR_FAILED_REPLY;
>>                         return_error_param = ret;
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 97cd7471e572..85eb87f6f92d 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -291,6 +291,7 @@ extern int  audit_alloc(struct task_struct *task);
>>  extern void __audit_free(struct task_struct *task);
>>  extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
>>  extern void audit_free_context(struct audit_context *context);
>> +extern void audit_free_local(struct audit_context *context);
>>  extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>>                                   unsigned long a2, unsigned long a3);
>>  extern void __audit_syscall_exit(int ret_success, long ret_value);
>> @@ -386,6 +387,19 @@ static inline void audit_ptrace(struct task_struct *t)
>>                 __audit_ptrace(t);
>>  }
>>
>> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
>> +{
>> +       struct audit_context *context = audit_context();
>> +
>> +       if (context)
>> +               return context;
>> +
>> +       if (lsm_multiple_contexts())
>> +               return audit_alloc_local(gfp);
>> +
>> +       return NULL;
>> +}
> We don't want to do this, at least not as it is written above.  We
> shouldn't have a function which abstracts away the creation of a local
> audit_context.  Usage of a local audit_context should be explicit in
> the caller, and if the caller's execution context is ambiguous enough
> that it can require both a task_struct audit_context and a local
> audit_context then we need to handle that on a case-by-case basis.
> Hiding it like this is guaranteed to cause problems later.

OK. Please understand that *every case* where I've used audit_alloc_for_lsm()
is a case where I have *verified* that context may be NULL. I will make
the change.

> I probably did a poor job of explaining what a local context is during
> the last patchset; I'll try to do a better job here but also let me
> say this as clear as I can ... if the "current" task struct is valid
> for a given code path, *never* use a local audit context.

I probably did a poor job of demonstrating that I never use a local
context where there's a valid current context.

>   The local
> audit context is a hack that is made necessary by the fact that we
> have to audit things which happen outside the scope of an executing
> task, e.g. the netfilter audit hooks, it should *never* be used when
> there is a valid task_struct.

In the existing audit code a "current context" is only needed for
syscall events, so that's the only case where it's allocated. Would
you suggest that I track down the non-syscall events that include
subj= fields and add allocate a "current context" for them? I looked
into doing that, and it wouldn't be simple.

> It's the audit_context which helps bind multiple audit records into a
> single event, creating a new, "local" audit_context destroys that
> binding

... if there's a current context. There often isn't. That's why I'm
using a local context. There is not a "current" context available.

>  as audit records created using that local audit_context have a
> different timestamp from those records created using the current
> task_struct's audit_context.

(Weeps) I only introduce a local context where there is no current
context available, so this is never a problem.

> Hopefully that makes sense?

Yes, it makes sense. Methinks you may believe that the current context
is available more regularly than it actually is.

I instrumented the audit event functions with:

	WARN_ONCE(audit_context, "%s has context\n", __func__);
	WARN_ONCE(!audit_context, "%s lacks context\n", __func__);

I only used local contexts where the 2nd WARN_ONCE was hit.


>>                                 /* Private API (for audit.c only) */
>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
>> @@ -560,6 +574,8 @@ extern int audit_signals;
>>  }
>>  static inline void audit_free_context(struct audit_context *context)
>>  { }
>> +static inline void audit_free_local(struct audit_context *context)
>> +{ }
>>  static inline int audit_alloc(struct task_struct *task)
>>  {
>>         return 0;
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 3e9743118fb9..b3cf68cf2bd6 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -182,6 +182,8 @@ struct lsmblob {
>>  #define LSMBLOB_INVALID                -1      /* Not a valid LSM slot number */
>>  #define LSMBLOB_NEEDED         -2      /* Slot requested on initialization */
>>  #define LSMBLOB_NOT_NEEDED     -3      /* Slot not requested */
>> +#define LSMBLOB_DISPLAY                -4      /* Use the "display" slot */
>> +#define LSMBLOB_FIRST          -5      /* Use the default "display" slot */
>>
>>  /**
>>   * lsmblob_init - initialize an lsmblob structure
>> @@ -248,6 +250,15 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
>>         return 0;
>>  }
>>
>> +static inline bool lsm_multiple_contexts(void)
>> +{
>> +#ifdef CONFIG_SECURITY
>> +       return lsm_slot_to_name(1) != NULL;
>> +#else
>> +       return false;
>> +#endif
>> +}
>> +
>>  /* These functions are in security/commoncap.c */
>>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>>                        int cap, unsigned int opts);
>> @@ -578,7 +589,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>                          size_t size);
>>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>>  int security_ismaclabel(const char *name);
>> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
>> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
>> +                            int display);
>>  int security_secctx_to_secid(const char *secdata, u32 seclen,
>>                              struct lsmblob *blob);
>>  void security_release_secctx(struct lsmcontext *cp);
>> @@ -1433,7 +1445,7 @@ static inline int security_ismaclabel(const char *name)
>>  }
>>
>>  static inline int security_secid_to_secctx(struct lsmblob *blob,
>> -                                          struct lsmcontext *cp)
>> +                                          struct lsmcontext *cp, int display)
>>  {
>>         return -EOPNOTSUPP;
>>  }
>> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
>> index 73fc25b4042b..216cb1ffc8f0 100644
>> --- a/include/net/netlabel.h
>> +++ b/include/net/netlabel.h
>> @@ -97,7 +97,7 @@ struct calipso_doi;
>>
>>  /* NetLabel audit information */
>>  struct netlbl_audit {
>> -       u32 secid;
>> +       struct lsmblob lsmdata;
>>         kuid_t loginuid;
>>         unsigned int sessionid;
>>  };
> This chunk seems lost here, does it belong in another patch?

Probably. I am getting a touch of patch-rot showing up.


>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index b77a52f93389..f4d567d4885e 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>>                  * and the infrastructure will know which it is.
>>                  */
>>                 lsmblob_init(&lb, scm->secid);
>> -               err = security_secid_to_secctx(&lb, &context);
>> +               err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
> Misplaced code change?

Same here.


>>                 if (!err) {
>>                         put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index cbff7c2a9724..a10fa01f7bf4 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -660,13 +660,22 @@ struct xfrm_spi_skb_cb {
>>  #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
>>
>>  #ifdef CONFIG_AUDITSYSCALL
>> -static inline struct audit_buffer *xfrm_audit_start(const char *op)
>> +static inline struct audit_buffer *xfrm_audit_start(const char *op,
>> +                                                   struct audit_context **lac)
>>  {
>> +       struct audit_context *context;
>>         struct audit_buffer *audit_buf = NULL;
>>
>>         if (audit_enabled == AUDIT_OFF)
>>                 return NULL;
>> -       audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
>> +       context = audit_context();
>> +       if (lac != NULL) {
>> +               if (lsm_multiple_contexts() && context == NULL)
>> +                       context = audit_alloc_local(GFP_ATOMIC);
>> +               *lac = context;
>> +       }
>> +
>> +       audit_buf = audit_log_start(context, GFP_ATOMIC,
>>                                     AUDIT_MAC_IPSEC_EVENT);
>>         if (audit_buf == NULL)
>>                 return NULL;
> Related to the other comments around local audit_contexts, we don't
> want to do this; use the existing audit_context, @lac in this case, so
> that this audit record is bound to the other associated records into a
> single audit event (all have the same timestamp).

Hmm. This is clearly a problem. Looks like a change came in
that I didn't see.

>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 841123390d41..cba63789a164 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -386,10 +386,12 @@ void audit_log_lost(const char *message)
>>  static int audit_log_config_change(char *function_name, u32 new, u32 old,
>>                                    int allow_changes)
>>  {
>> +       struct audit_context *context;
>>         struct audit_buffer *ab;
>>         int rc = 0;
>>
>> -       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>> +       context = audit_alloc_for_lsm(GFP_KERNEL);
>> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> We shouldn't need to do this for the reasons discussed up near the top
> of this email (and elsewhere as well).

Here and elsewhere, I only put audit_alloc_for_lsm() in because there
are cases where audit_context() returns NULL. Really.

>
> I'm going to refrain from commenting on the other uses of
> audit_alloc_for_lsm() in this patch unless there is something unique
> to the code path, so if you don't see me comment about a use of
> audit_alloc_for_lsm() you can assume it should be removed and the
> existing audit_context used instead.
>
>> @@ -399,6 +401,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>>                 allow_changes = 0; /* Something weird, deny request */
>>         audit_log_format(ab, " res=%d", allow_changes);
>>         audit_log_end(ab);
>> +       audit_free_local(context);
> See my comment directly above regarding usage of
> audit_alloc_for_lsm(), it obviously applies here too.
>
>> @@ -2128,6 +2136,36 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>>                 audit_log_format(ab, "(null)");
>>  }
>>
>> +static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)
> See my note below about moving this into audit_log_task_context(),

Either works for me. This seemed consistent with the rest of the audit
code, but I'm happy to change it if you like that better.

>  but
> if we really need to keep this as a separate function, let's consider
> changing the name to something which indicates that it logs the LSM
> data as *subject* fields.  How about audit_log_lsm_subj()?
>
>> +{
>> +       struct audit_buffer *ab;
>> +       struct lsmcontext lsmdata;
>> +       bool sep = false;
>> +       int error;
>> +       int i;
>> +
>> +       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
>> +       if (!ab)
>> +               return; /* audit_panic or being filtered */
>> +
>> +       for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>> +               if (blob->secid[i] == 0)
>> +                       continue;
>> +               error = security_secid_to_secctx(blob, &lsmdata, i);
>> +               if (error && error != -EINVAL) {
>> +                       audit_panic("error in audit_log_lsm");
>> +                       return;
>> +               }
>> +
>> +               audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
>> +                                lsm_slot_to_name(i), lsmdata.context);
>> +               sep = true;
> Since @i starts at zero, you can get rid of @sep by replacing it with @i:
>
>   audit_log_format(ab, ..., (i ? " " : ""), ...);

Clever.

>
>> +               security_release_secctx(&lsmdata);
>> +       }
>> +       audit_log_end(ab);
>> +}
>> @@ -2138,7 +2176,18 @@ int audit_log_task_context(struct audit_buffer *ab)
>>         if (!lsmblob_is_set(&blob))
>>                 return 0;
>>
>> -       error = security_secid_to_secctx(&blob, &context);
>> +       /*
>> +        * If there is more than one security module that has a
>> +        * subject "context" it's necessary to put the subject data
>> +        * into a separate record to maintain compatibility.
>> +        */
>> +       if (lsm_multiple_contexts()) {
>> +               audit_log_format(ab, " subj=?");
>> +               audit_log_lsm(ab->ctx, &blob);
>> +               return 0;
>> +       }
>> +
>> +       error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> Instead of the lsm_multiple_contexts() case bailing on the rest of the
> function with a return inside the if-block, let's made the code a bit
> more robust by organizing it like this:

Sure, why not?

>
>   int audit_log_task_context(ab)
>   {
>      /* common stuff at the start */
>
>      if (lsm_multiple_contexts()) {
>        /* multi-LSM stuff */
>      } else {
>        /* single LSM stuff */
>      }
>
>      /* common stuff at the end */
>   }
>
> ... it also may make sense to just move the body of audit_log_lsm()
> into audit_log_task_context() and do away with audit_log_lsm()
> entirely; is it ever going to be called from anywhere else?
>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 0e58a3ab56f5..01fdcbf468c0 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -993,12 +993,11 @@ struct audit_context *audit_alloc_local(gfp_t gfpflags)
>>         context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
>>         if (!context) {
>>                 audit_log_lost("out of memory in audit_alloc_local");
>> -               goto out;
>> +               return NULL;
>>         }
>>         context->serial = audit_serial();
>>         ktime_get_coarse_real_ts64(&context->ctime);
>>         context->local = true;
>> -out:
>>         return context;
>>  }
> This chunk should be moved to 21/25 when audit_alloc_local() is first defined.

True. I was trying to minimize the change to the original audit_alloc_local()
patch on the assumption that it was coming in for other reasons, but that hasn't
happened.


>> @@ -1019,6 +1018,13 @@ void audit_free_context(struct audit_context *context)
>>  }
>>  EXPORT_SYMBOL(audit_free_context);
>>
>> +void audit_free_local(struct audit_context *context)
>> +{
>> +       if (context && context->local)
>> +               audit_free_context(context);
>> +}
>> +EXPORT_SYMBOL(audit_free_local);
> If this is strictly necessary, and I don't think it is, it should also
> be moved to patch 21/25 with the original definition of a local
> audit_context.  However,  there really should be no reason why we have
> to distinguish between a proper and local audtit_context when it comes
> to free'ing the memory, just call audit_free_context() in both cases.
>
>> @@ -1036,7 +1042,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>>                          from_kuid(&init_user_ns, auid),
>>                          from_kuid(&init_user_ns, uid), sessionid);
>>         if (lsmblob_is_set(blob)) {
>> -               if (security_secid_to_secctx(blob, &lsmctx)) {
>> +               if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
> Misplaced code change?
>
> Actually, there are a lot of these below, I'm not going to comment on
> all of them as I think you get the idea ... and I very well may be
> wrong so I'll save you all of my wrongness in that case :)
>
>> diff --git a/security/security.c b/security/security.c
>> index cb359e185d1a..5d7fd982f84a 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2309,7 +2309,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>                 hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>>                                      list) {
>>                         rc = hp->hook.setprocattr(name, value, size);
>> -                       if (rc < 0)
>> +                       if (rc < 0 && rc != -EINVAL)
>>                                 return rc;
>>                 }
> This really looks misplaced ... ?

Yeah, you're not the first person to notice these bits of patch-rot.
I have some clean-up to do.

Thank you for the review.



--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 13, 2021, 3:31 p.m. UTC | #4
On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/12/2021 1:59 PM, Paul Moore wrote:
> > On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Create a new audit record type to contain the subject information
> >> when there are multiple security modules that require such data.

...

> > The local
> > audit context is a hack that is made necessary by the fact that we
> > have to audit things which happen outside the scope of an executing
> > task, e.g. the netfilter audit hooks, it should *never* be used when
> > there is a valid task_struct.
>
> In the existing audit code a "current context" is only needed for
> syscall events, so that's the only case where it's allocated. Would
> you suggest that I track down the non-syscall events that include
> subj= fields and add allocate a "current context" for them? I looked
> into doing that, and it wouldn't be simple.

This is why the "local context" was created.  Prior to these stacking
additions, and the audit container ID work, we never needed to group
multiple audit records outside of a syscall context into a single
audit event so passing a NULL context into audit_log_start() was
reasonable.  The local context was designed as a way to generate a
context for use in a local function scope to group multiple records,
however, for reasons I'll get to below I'm now wondering if the local
context approach is really workable ...

> > Hopefully that makes sense?
>
> Yes, it makes sense. Methinks you may believe that the current context
> is available more regularly than it actually is.
>
> I instrumented the audit event functions with:
>
>         WARN_ONCE(audit_context, "%s has context\n", __func__);
>         WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
>
> I only used local contexts where the 2nd WARN_ONCE was hit.

What does your audit config look like?  Both the kernel command line
and the output of 'auditctl -l' would be helpful.

I'm beginning to suspect that you have the default
we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
audit configuration that is de rigueur for many distros these days.
If that is the case, there are many cases where you would not see a
NULL current->audit_context simply because the config never allocated
one, see kernel/auditsc.c:audit_alloc().  If that is the case, I'm
honestly a little surprised we didn't realize that earlier, especially
given all the work/testing that Richard has done with the audit
container ID bits, but then again he surely had a proper audit config
during his testing so it wouldn't have appeared.

Good times.

Regardless, assuming that is the case we probably need to find an
alternative to the local context approach as it currently works.  For
reasons we already talked about, we don't want to use a local
audit_context if there is the possibility for a proper
current->audit_context, but we need to do *something* so that we can
group these multiple events into a single record.

Since this is just occurring to me now I need a bit more time to think
on possible solutions - all good ideas are welcome - but the first
thing that pops into my head is that we need to augment
audit_log_end() to potentially generated additional, associated
records similar to what we do on syscall exit in audit_log_exit().  Of
course the audit_log_end() changes would be much more limited than
audit_log_exit(), just the LSM subject and audit container ID info,
and even then we might want to limit that to cases where the ab->ctx
value is NULL and let audit_log_exit() handle it otherwise.  We may
need to store the event type in the audit_buffer during
audit_log_start() so that we can later use that in audit_log_end() to
determine what additional records are needed.

Regardless, let's figure out why all your current->audit_context
values are NULL first (report back on your audit config please), I may
be worrying about a hypothetical that isn't real.
Casey Schaufler Aug. 13, 2021, 6:48 p.m. UTC | #5
On 8/13/2021 8:31 AM, Paul Moore wrote:
> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/12/2021 1:59 PM, Paul Moore wrote:
>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Create a new audit record type to contain the subject information
>>>> when there are multiple security modules that require such data.
> ...
>
>>> The local
>>> audit context is a hack that is made necessary by the fact that we
>>> have to audit things which happen outside the scope of an executing
>>> task, e.g. the netfilter audit hooks, it should *never* be used when
>>> there is a valid task_struct.
>> In the existing audit code a "current context" is only needed for
>> syscall events, so that's the only case where it's allocated. Would
>> you suggest that I track down the non-syscall events that include
>> subj= fields and add allocate a "current context" for them? I looked
>> into doing that, and it wouldn't be simple.
> This is why the "local context" was created.  Prior to these stacking
> additions, and the audit container ID work, we never needed to group
> multiple audit records outside of a syscall context into a single
> audit event so passing a NULL context into audit_log_start() was
> reasonable.  The local context was designed as a way to generate a
> context for use in a local function scope to group multiple records,
> however, for reasons I'll get to below I'm now wondering if the local
> context approach is really workable ...

I haven't found a place where it didn't work. What is the concern?


>>> Hopefully that makes sense?
>> Yes, it makes sense. Methinks you may believe that the current context
>> is available more regularly than it actually is.
>>
>> I instrumented the audit event functions with:
>>
>>         WARN_ONCE(audit_context, "%s has context\n", __func__);
>>         WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
>>
>> I only used local contexts where the 2nd WARN_ONCE was hit.
> What does your audit config look like?  Both the kernel command line
> and the output of 'auditctl -l' would be helpful.

On the fedora system:

BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug

-a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW

On the Ubuntu system:

BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug

No rules

> I'm beginning to suspect that you have the default
> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> audit configuration that is de rigueur for many distros these days.

Yes, but I've also fiddled about with it so as to get better event coverage.
I've run the audit-testsuite, which has got to fiddle about with the audit
configuration.

> If that is the case, there are many cases where you would not see a
> NULL current->audit_context simply because the config never allocated
> one, see kernel/auditsc.c:audit_alloc().

I assume you mean that I *would* see a NULL current->audit_context
in the "event not enabled" case.
 

> If that is the case, I'm
> honestly a little surprised we didn't realize that earlier, especially
> given all the work/testing that Richard has done with the audit
> container ID bits, but then again he surely had a proper audit config
> during his testing so it wouldn't have appeared.
>
> Good times.

Indeed.

> Regardless, assuming that is the case we probably need to find an
> alternative to the local context approach as it currently works.  For
> reasons we already talked about, we don't want to use a local
> audit_context if there is the possibility for a proper
> current->audit_context, but we need to do *something* so that we can
> group these multiple events into a single record.

I tried a couple things, but neither was satisfactory.

> Since this is just occurring to me now I need a bit more time to think
> on possible solutions - all good ideas are welcome - but the first
> thing that pops into my head is that we need to augment
> audit_log_end() to potentially generated additional, associated
> records similar to what we do on syscall exit in audit_log_exit().

I looked into that. You need a place to save the timestamp
that doesn't disappear. That's the role the audit_context plays
now.

>   Of
> course the audit_log_end() changes would be much more limited than
> audit_log_exit(), just the LSM subject and audit container ID info,
> and even then we might want to limit that to cases where the ab->ctx
> value is NULL and let audit_log_exit() handle it otherwise.  We may
> need to store the event type in the audit_buffer during
> audit_log_start() so that we can later use that in audit_log_end() to
> determine what additional records are needed.
>
> Regardless, let's figure out why all your current->audit_context
> values are NULL

That's what's maddening, and why I implemented audit_alloc_for_lsm().
They aren't all NULL. Sometimes current->audit_context is NULL,
sometimes it isn't, for the same event. I thought it might be a
question of the netlink interface being treated specially, but
that doesn't explain all the cases.

>  first (report back on your audit config please), I may
> be worrying about a hypothetical that isn't real.
>


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 13, 2021, 8:43 p.m. UTC | #6
On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/13/2021 8:31 AM, Paul Moore wrote:
> > On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/12/2021 1:59 PM, Paul Moore wrote:
> >>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> Create a new audit record type to contain the subject information
> >>>> when there are multiple security modules that require such data.
> > ...
> >
> >>> The local
> >>> audit context is a hack that is made necessary by the fact that we
> >>> have to audit things which happen outside the scope of an executing
> >>> task, e.g. the netfilter audit hooks, it should *never* be used when
> >>> there is a valid task_struct.
> >> In the existing audit code a "current context" is only needed for
> >> syscall events, so that's the only case where it's allocated. Would
> >> you suggest that I track down the non-syscall events that include
> >> subj= fields and add allocate a "current context" for them? I looked
> >> into doing that, and it wouldn't be simple.
> >
> > This is why the "local context" was created.  Prior to these stacking
> > additions, and the audit container ID work, we never needed to group
> > multiple audit records outside of a syscall context into a single
> > audit event so passing a NULL context into audit_log_start() was
> > reasonable.  The local context was designed as a way to generate a
> > context for use in a local function scope to group multiple records,
> > however, for reasons I'll get to below I'm now wondering if the local
> > context approach is really workable ...
>
> I haven't found a place where it didn't work. What is the concern?

The concern is that use of a local context can destroy any hopes of
linking with other related records, e.g. SYSCALL and PATH records, to
form a single cohesive event.  If the current task_struct is valid for
a given function invocation then we *really* should be using current's
audit_context.

However, based on our discussion here it would seem that we may have
some issues where current->audit_context is not being managed
correctly.  I'm not surprised, but I will admit to being disappointed.

> > What does your audit config look like?  Both the kernel command line
> > and the output of 'auditctl -l' would be helpful.
>
> On the fedora system:
>
> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
>
> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
>
> On the Ubuntu system:
>
> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
>
> No rules

The Fedora system looks to have some audit-testsuite leftovers, but
that shouldn't have an impact on what we are discussing; in both cases
I would expect current->audit_context to be allocated and non-NULL.

> > I'm beginning to suspect that you have the default
> > we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> > audit configuration that is de rigueur for many distros these days.
>
> Yes, but I've also fiddled about with it so as to get better event coverage.
> I've run the audit-testsuite, which has got to fiddle about with the audit
> configuration.

Yes, it looks like my hunch was wrong.

> > If that is the case, there are many cases where you would not see a
> > NULL current->audit_context simply because the config never allocated
> > one, see kernel/auditsc.c:audit_alloc().
>
> I assume you mean that I *would* see a NULL current->audit_context
> in the "event not enabled" case.

Yep, typo.

> > Regardless, assuming that is the case we probably need to find an
> > alternative to the local context approach as it currently works.  For
> > reasons we already talked about, we don't want to use a local
> > audit_context if there is the possibility for a proper
> > current->audit_context, but we need to do *something* so that we can
> > group these multiple events into a single record.
>
> I tried a couple things, but neither was satisfactory.
>
> > Since this is just occurring to me now I need a bit more time to think
> > on possible solutions - all good ideas are welcome - but the first
> > thing that pops into my head is that we need to augment
> > audit_log_end() to potentially generated additional, associated
> > records similar to what we do on syscall exit in audit_log_exit().
>
> I looked into that. You need a place to save the timestamp
> that doesn't disappear. That's the role the audit_context plays
> now.

Yes, I've spent a few hours staring at the poorly planned struct that
is audit_context ;)

Regardless, the obvious place for such a thing is audit_buffer; we can
stash whatever we need in there.

> >  Of
> > course the audit_log_end() changes would be much more limited than
> > audit_log_exit(), just the LSM subject and audit container ID info,
> > and even then we might want to limit that to cases where the ab->ctx
> > value is NULL and let audit_log_exit() handle it otherwise.  We may
> > need to store the event type in the audit_buffer during
> > audit_log_start() so that we can later use that in audit_log_end() to
> > determine what additional records are needed.
> >
> > Regardless, let's figure out why all your current->audit_context
> > values are NULL
>
> That's what's maddening, and why I implemented audit_alloc_for_lsm().
> They aren't all NULL. Sometimes current->audit_context is NULL,
> sometimes it isn't, for the same event. I thought it might be a
> question of the netlink interface being treated specially, but
> that doesn't explain all the cases.

Your netlink changes are exactly what made me think, "this is
obviously wrong", but now I'm wondering if a previously held
assumption of "current is valid and points to the calling process" in
the case of the kernel servicing netlink messages sent from userspace.
Or rather, perhaps that assumption is still true but something is
causing current->audit_context to be NULL in that case.

Friday the 13th indeed.
Casey Schaufler Aug. 13, 2021, 9:47 p.m. UTC | #7
On 8/13/2021 1:43 PM, Paul Moore wrote:
> On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/13/2021 8:31 AM, Paul Moore wrote:
>>> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 8/12/2021 1:59 PM, Paul Moore wrote:
>>>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> Create a new audit record type to contain the subject information
>>>>>> when there are multiple security modules that require such data.
>>> ...
>>>
>>>>> The local
>>>>> audit context is a hack that is made necessary by the fact that we
>>>>> have to audit things which happen outside the scope of an executing
>>>>> task, e.g. the netfilter audit hooks, it should *never* be used when
>>>>> there is a valid task_struct.
>>>> In the existing audit code a "current context" is only needed for
>>>> syscall events, so that's the only case where it's allocated. Would
>>>> you suggest that I track down the non-syscall events that include
>>>> subj= fields and add allocate a "current context" for them? I looked
>>>> into doing that, and it wouldn't be simple.
>>> This is why the "local context" was created.  Prior to these stacking
>>> additions, and the audit container ID work, we never needed to group
>>> multiple audit records outside of a syscall context into a single
>>> audit event so passing a NULL context into audit_log_start() was
>>> reasonable.  The local context was designed as a way to generate a
>>> context for use in a local function scope to group multiple records,
>>> however, for reasons I'll get to below I'm now wondering if the local
>>> context approach is really workable ...
>> I haven't found a place where it didn't work. What is the concern?
> The concern is that use of a local context can destroy any hopes of
> linking with other related records, e.g. SYSCALL and PATH records, to
> form a single cohesive event.  If the current task_struct is valid for
> a given function invocation then we *really* should be using current's
> audit_context.
>
> However, based on our discussion here it would seem that we may have
> some issues where current->audit_context is not being managed
> correctly.  I'm not surprised, but I will admit to being disappointed.

I'd believe that with syscall audit being a special case for other reasons
the multiple record situation got taken care of on a case-by-case basis
and no one really paid much attention to generality. It's understandable.

>>> What does your audit config look like?  Both the kernel command line
>>> and the output of 'auditctl -l' would be helpful.
>> On the fedora system:
>>
>> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
>> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
>> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
>>
>> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
>>
>> On the Ubuntu system:
>>
>> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
>> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
>>
>> No rules
> The Fedora system looks to have some audit-testsuite leftovers, but
> that shouldn't have an impact on what we are discussing; in both cases
> I would expect current->audit_context to be allocated and non-NULL.

As would I.


>>> I'm beginning to suspect that you have the default
>>> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
>>> audit configuration that is de rigueur for many distros these days.
>> Yes, but I've also fiddled about with it so as to get better event coverage.
>> I've run the audit-testsuite, which has got to fiddle about with the audit
>> configuration.
> Yes, it looks like my hunch was wrong.
>
>>> If that is the case, there are many cases where you would not see a
>>> NULL current->audit_context simply because the config never allocated
>>> one, see kernel/auditsc.c:audit_alloc().
>> I assume you mean that I *would* see a NULL current->audit_context
>> in the "event not enabled" case.
> Yep, typo.
>
>>> Regardless, assuming that is the case we probably need to find an
>>> alternative to the local context approach as it currently works.  For
>>> reasons we already talked about, we don't want to use a local
>>> audit_context if there is the possibility for a proper
>>> current->audit_context, but we need to do *something* so that we can
>>> group these multiple events into a single record.
>> I tried a couple things, but neither was satisfactory.
>>
>>> Since this is just occurring to me now I need a bit more time to think
>>> on possible solutions - all good ideas are welcome - but the first
>>> thing that pops into my head is that we need to augment
>>> audit_log_end() to potentially generated additional, associated
>>> records similar to what we do on syscall exit in audit_log_exit().
>> I looked into that. You need a place to save the timestamp
>> that doesn't disappear. That's the role the audit_context plays
>> now.
> Yes, I've spent a few hours staring at the poorly planned struct that
> is audit_context ;)
>
> Regardless, the obvious place for such a thing is audit_buffer; we can
> stash whatever we need in there.

I had considered doing that, but was afraid that moving the timestamp
out of the audit_context might have dire consequences.


>>>  Of
>>> course the audit_log_end() changes would be much more limited than
>>> audit_log_exit(), just the LSM subject and audit container ID info,
>>> and even then we might want to limit that to cases where the ab->ctx
>>> value is NULL and let audit_log_exit() handle it otherwise.  We may
>>> need to store the event type in the audit_buffer during
>>> audit_log_start() so that we can later use that in audit_log_end() to
>>> determine what additional records are needed.
>>>
>>> Regardless, let's figure out why all your current->audit_context
>>> values are NULL
>> That's what's maddening, and why I implemented audit_alloc_for_lsm().
>> They aren't all NULL. Sometimes current->audit_context is NULL,
>> sometimes it isn't, for the same event. I thought it might be a
>> question of the netlink interface being treated specially, but
>> that doesn't explain all the cases.
> Your netlink changes are exactly what made me think, "this is
> obviously wrong", but now I'm wondering if a previously held
> assumption of "current is valid and points to the calling process" in
> the case of the kernel servicing netlink messages sent from userspace.

If that's the case the subject data in the audit record is going
to be bogus. From what I've seen that data appears to be correct.

> Or rather, perhaps that assumption is still true but something is
> causing current->audit_context to be NULL in that case.

I can imagine someone deciding not to set up audit_context in
situations like netlink because they knew that nothing following
that would be a syscall event. I've been looking into the audit
userspace and there are assumptions like that all over the place.

> Friday the 13th indeed.

I've been banging my head against this for a couple months.
My biggest fear is that I may have learned enough about the
audit system to make useful contributions. 




--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 16, 2021, 6:57 p.m. UTC | #8
On Fri, Aug 13, 2021 at 5:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/13/2021 1:43 PM, Paul Moore wrote:
> > On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/13/2021 8:31 AM, Paul Moore wrote:
> >>> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> On 8/12/2021 1:59 PM, Paul Moore wrote:
> >>>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>> Create a new audit record type to contain the subject information
> >>>>>> when there are multiple security modules that require such data.
> >>> ...
> >>>
> >>>>> The local
> >>>>> audit context is a hack that is made necessary by the fact that we
> >>>>> have to audit things which happen outside the scope of an executing
> >>>>> task, e.g. the netfilter audit hooks, it should *never* be used when
> >>>>> there is a valid task_struct.
> >>>> In the existing audit code a "current context" is only needed for
> >>>> syscall events, so that's the only case where it's allocated. Would
> >>>> you suggest that I track down the non-syscall events that include
> >>>> subj= fields and add allocate a "current context" for them? I looked
> >>>> into doing that, and it wouldn't be simple.
> >>> This is why the "local context" was created.  Prior to these stacking
> >>> additions, and the audit container ID work, we never needed to group
> >>> multiple audit records outside of a syscall context into a single
> >>> audit event so passing a NULL context into audit_log_start() was
> >>> reasonable.  The local context was designed as a way to generate a
> >>> context for use in a local function scope to group multiple records,
> >>> however, for reasons I'll get to below I'm now wondering if the local
> >>> context approach is really workable ...
> >> I haven't found a place where it didn't work. What is the concern?
> > The concern is that use of a local context can destroy any hopes of
> > linking with other related records, e.g. SYSCALL and PATH records, to
> > form a single cohesive event.  If the current task_struct is valid for
> > a given function invocation then we *really* should be using current's
> > audit_context.
> >
> > However, based on our discussion here it would seem that we may have
> > some issues where current->audit_context is not being managed
> > correctly.  I'm not surprised, but I will admit to being disappointed.
>
> I'd believe that with syscall audit being a special case for other reasons
> the multiple record situation got taken care of on a case-by-case basis
> and no one really paid much attention to generality. It's understandable.
>
> >>> What does your audit config look like?  Both the kernel command line
> >>> and the output of 'auditctl -l' would be helpful.
> >> On the fedora system:
> >>
> >> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
> >> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
> >> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
> >>
> >> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
> >>
> >> On the Ubuntu system:
> >>
> >> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
> >> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
> >>
> >> No rules
> > The Fedora system looks to have some audit-testsuite leftovers, but
> > that shouldn't have an impact on what we are discussing; in both cases
> > I would expect current->audit_context to be allocated and non-NULL.
>
> As would I.
>
>
> >>> I'm beginning to suspect that you have the default
> >>> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> >>> audit configuration that is de rigueur for many distros these days.
> >> Yes, but I've also fiddled about with it so as to get better event coverage.
> >> I've run the audit-testsuite, which has got to fiddle about with the audit
> >> configuration.
> > Yes, it looks like my hunch was wrong.
> >
> >>> If that is the case, there are many cases where you would not see a
> >>> NULL current->audit_context simply because the config never allocated
> >>> one, see kernel/auditsc.c:audit_alloc().
> >> I assume you mean that I *would* see a NULL current->audit_context
> >> in the "event not enabled" case.
> > Yep, typo.
> >
> >>> Regardless, assuming that is the case we probably need to find an
> >>> alternative to the local context approach as it currently works.  For
> >>> reasons we already talked about, we don't want to use a local
> >>> audit_context if there is the possibility for a proper
> >>> current->audit_context, but we need to do *something* so that we can
> >>> group these multiple events into a single record.
> >> I tried a couple things, but neither was satisfactory.
> >>
> >>> Since this is just occurring to me now I need a bit more time to think
> >>> on possible solutions - all good ideas are welcome - but the first
> >>> thing that pops into my head is that we need to augment
> >>> audit_log_end() to potentially generated additional, associated
> >>> records similar to what we do on syscall exit in audit_log_exit().
> >> I looked into that. You need a place to save the timestamp
> >> that doesn't disappear. That's the role the audit_context plays
> >> now.
> > Yes, I've spent a few hours staring at the poorly planned struct that
> > is audit_context ;)
> >
> > Regardless, the obvious place for such a thing is audit_buffer; we can
> > stash whatever we need in there.
>
> I had considered doing that, but was afraid that moving the timestamp
> out of the audit_context might have dire consequences.

Don't move, copy.  If there is a valid context one would stash the
timestamp there, if not, we stash it in the audit_buffer.  However,
before we start messing with that too much I would like to better
understand why we aren't seeing a valid audit_context in the netlink
case at the very least.

> >>>  Of
> >>> course the audit_log_end() changes would be much more limited than
> >>> audit_log_exit(), just the LSM subject and audit container ID info,
> >>> and even then we might want to limit that to cases where the ab->ctx
> >>> value is NULL and let audit_log_exit() handle it otherwise.  We may
> >>> need to store the event type in the audit_buffer during
> >>> audit_log_start() so that we can later use that in audit_log_end() to
> >>> determine what additional records are needed.
> >>>
> >>> Regardless, let's figure out why all your current->audit_context
> >>> values are NULL
> >> That's what's maddening, and why I implemented audit_alloc_for_lsm().
> >> They aren't all NULL. Sometimes current->audit_context is NULL,
> >> sometimes it isn't, for the same event. I thought it might be a
> >> question of the netlink interface being treated specially, but
> >> that doesn't explain all the cases.
> >
> > Your netlink changes are exactly what made me think, "this is
> > obviously wrong", but now I'm wondering if a previously held
> > assumption of "current is valid and points to the calling process" in
> > the case of the kernel servicing netlink messages sent from userspace.
>
> If that's the case the subject data in the audit record is going
> to be bogus. From what I've seen that data appears to be correct.

Yeah, the thought occurred to me, but we are clearly already in the
maybe-the-assumptions-are-wrong stage so I'm not going to rely on that
being 100%.  We definitely need to track this down before we start
making to many more guesses about what is working and what is not.

> > Or rather, perhaps that assumption is still true but something is
> > causing current->audit_context to be NULL in that case.
>
> I can imagine someone deciding not to set up audit_context in
> situations like netlink because they knew that nothing following
> that would be a syscall event.

*If* the user/kernel transition happens as part of the netlink socket
send/write/etc. syscall then it *should* have all of the audit syscall
setup already done ... however, see my earlier comments about
assumptions :/

> I've been looking into the audit
> userspace and there are assumptions like that all over the place.

I've made my feelings about audit's design known quite a bit already
so I'm not going to drag all of that back up, all I'll say is that I
believe the audit design was tragically and inherently flawed in many
ways.  We've been working to resolve some of those issues in the
kernel for a little while now, but the audit userspace remains rooted
in some of those original design decisions.  Of course, these are just
my opinions, others clearly feel differently.

Regardless, and somewhat independent of our discussion here, now that
I am back to being able to dedicate a good chunk of my time to
upstream efforts, one of my priorities is to start repairing audit ...
however, I need to get past the io_uring mess first.

> > Friday the 13th indeed.
>
> I've been banging my head against this for a couple months.
> My biggest fear is that I may have learned enough about the
> audit system to make useful contributions.

As the usual refrain goes, "patches are always welcome" ... and I say
that with equal parts honesty and warning :)

Even if you aren't comfortable putting together a patch, simply
root-causing the missing audit_context setup in the netlink case would
be helpful.
Casey Schaufler Aug. 18, 2021, 9:59 p.m. UTC | #9
On 8/16/2021 11:57 AM, Paul Moore wrote:
> On Fri, Aug 13, 2021 at 5:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/13/2021 1:43 PM, Paul Moore wrote:
...
> Yeah, the thought occurred to me, but we are clearly already in the
> maybe-the-assumptions-are-wrong stage so I'm not going to rely on that
> being 100%.  We definitely need to track this down before we start
> making to many more guesses about what is working and what is not.

I've been tracking down where the audit context isn't set where
we'd expect it to be, I've identified 5 cases:

	1000	AUDIT_GET 		- Get Status
	1001	AUDIT_SET 		- Set status enable/disable/auditd
	1010	AUDIT_SIGNAL_INFO
	1130	AUDIT_SERVICE_START
	1131	AUDIT_SEVICE_STOP

These are all events that relate to the audit system itself.
It seems plausible that these really aren't syscalls and hence
shouldn't be expected to have an audit_context. I will create a
patch that treats these as the special cases I believe them to be.



--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 19, 2021, 12:47 a.m. UTC | #10
On Wed, Aug 18, 2021 at 5:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 8/16/2021 11:57 AM, Paul Moore wrote:
> > On Fri, Aug 13, 2021 at 5:47 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/13/2021 1:43 PM, Paul Moore wrote:
> ...
> > Yeah, the thought occurred to me, but we are clearly already in the
> > maybe-the-assumptions-are-wrong stage so I'm not going to rely on that
> > being 100%.  We definitely need to track this down before we start
> > making to many more guesses about what is working and what is not.
>
> I've been tracking down where the audit context isn't set where
> we'd expect it to be, I've identified 5 cases:
>
>         1000    AUDIT_GET               - Get Status
>         1001    AUDIT_SET               - Set status enable/disable/auditd
>         1010    AUDIT_SIGNAL_INFO
>         1130    AUDIT_SERVICE_START
>         1131    AUDIT_SEVICE_STOP
>
> These are all events that relate to the audit system itself.
> It seems plausible that these really aren't syscalls and hence
> shouldn't be expected to have an audit_context. I will create a
> patch that treats these as the special cases I believe them to be.

Yes, all but two of these could be considered to be audit subsystem
control messages, but AUDIT_SERVICE_{START,STOP} I think definitely
fall outside the audit subsystem control message category.  The
AUDIT_SERVICE_{START,STOP} records are used to indicate when a
service, e.g. NetworkManager, starts and stops; on my fedora test
system they are generated by systemd since it manages service state on
that system; a quick example is below, but I'm sure you've seen plenty
of these already.

% ausearch -m SERVICE_START
time->Wed Aug 18 20:13:00 2021
type=SERVICE_START msg=audit(1629331980.779:1186): pid=1 uid=0 auid=4294967295 s
es=4294967295 subj=system_u:system_r:init_t:s0 msg='unit=NetworkManager-dispatch
er comm="systemd" exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? re
s=success'

However, regardless of if the message is related to controlling the
audit subsystem or not, we do want to be able to associate those
records with other related records, e.g. SYSCALL records.  Looking at
the message types you listed above, they are all records that are
triggered by userspace via netlink messages; if you haven't already I
would start poking along that code path to see if something looks
awry.

I just spent a few minutes tracing the code paths up from audit
through netlink and then through the socket layer and I'm not seeing
anything obvious where the path differs from any other syscall;
current->audit_context *should* be valid just like any other syscall.
However, I do have to ask, are you only seeing these audit records
with a current->audit_context equal to NULL during early boot?
Casey Schaufler Aug. 19, 2021, 12:56 a.m. UTC | #11
On 8/18/2021 5:47 PM, Paul Moore wrote:
> ...
> I just spent a few minutes tracing the code paths up from audit
> through netlink and then through the socket layer and I'm not seeing
> anything obvious where the path differs from any other syscall;
> current->audit_context *should* be valid just like any other syscall.
> However, I do have to ask, are you only seeing these audit records
> with a current->audit_context equal to NULL during early boot?

Nope. Sorry.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Casey Schaufler Aug. 19, 2021, 10:41 p.m. UTC | #12
On 8/18/2021 5:56 PM, Casey Schaufler wrote:
> On 8/18/2021 5:47 PM, Paul Moore wrote:
>> ...
>> I just spent a few minutes tracing the code paths up from audit
>> through netlink and then through the socket layer and I'm not seeing
>> anything obvious where the path differs from any other syscall;
>> current->audit_context *should* be valid just like any other syscall.
>> However, I do have to ask, are you only seeing these audit records
>> with a current->audit_context equal to NULL during early boot?
> Nope. Sorry.

It looks as if all of the NULL audit_context cases are for either
auditd or systemd. Given what the events are, this isn't especially
surprising.



--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 20, 2021, 7:06 p.m. UTC | #13
On Thu, Aug 19, 2021 at 6:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/18/2021 5:56 PM, Casey Schaufler wrote:
> > On 8/18/2021 5:47 PM, Paul Moore wrote:
> >> ...
> >> I just spent a few minutes tracing the code paths up from audit
> >> through netlink and then through the socket layer and I'm not seeing
> >> anything obvious where the path differs from any other syscall;
> >> current->audit_context *should* be valid just like any other syscall.
> >> However, I do have to ask, are you only seeing these audit records
> >> with a current->audit_context equal to NULL during early boot?
> >
> > Nope. Sorry.
>
> It looks as if all of the NULL audit_context cases are for either
> auditd or systemd. Given what the events are, this isn't especially
> surprising.

I think we may be back to the "early boot" theory.

Unless you explicitly enable audit on the kernel cmdline, e.g.
"audit=1", processes started before userspace enables audit will not
have a properly allocated audit_context; see the "if
(likely(!audit_ever_enabled))" check at the top of audit_alloc() for
the reason why.

I could be wrong here, but I suspect if you add "audit=1" to your
kernel command line those remaining cases of NULL audit_contexts will
resolve themselves.  If not, we still have work to do ... well, I mean
we still have (different) work to do even if this solves the mystery,
it's just that we can now explain what you are seeing :)
Casey Schaufler Aug. 20, 2021, 7:17 p.m. UTC | #14
On 8/20/2021 12:06 PM, Paul Moore wrote:
> On Thu, Aug 19, 2021 at 6:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/18/2021 5:56 PM, Casey Schaufler wrote:
>>> On 8/18/2021 5:47 PM, Paul Moore wrote:
>>>> ...
>>>> I just spent a few minutes tracing the code paths up from audit
>>>> through netlink and then through the socket layer and I'm not seeing
>>>> anything obvious where the path differs from any other syscall;
>>>> current->audit_context *should* be valid just like any other syscall.
>>>> However, I do have to ask, are you only seeing these audit records
>>>> with a current->audit_context equal to NULL during early boot?
>>> Nope. Sorry.
>> It looks as if all of the NULL audit_context cases are for either
>> auditd or systemd. Given what the events are, this isn't especially
>> surprising.
> I think we may be back to the "early boot" theory.
>
> Unless you explicitly enable audit on the kernel cmdline, e.g.
> "audit=1", processes started before userspace enables audit will not
> have a properly allocated audit_context; see the "if
> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
> the reason why.
>
> I could be wrong here, but I suspect if you add "audit=1" to your
> kernel command line those remaining cases of NULL audit_contexts will
> resolve themselves.  If not, we still have work to do ... well, I mean
> we still have (different) work to do even if this solves the mystery,
> it's just that we can now explain what you are seeing :)

Yup, adding "audit=1" to the command line appears to have gotten
systemd an audit context. It looks like user space enabling audit
doesn't assign an audit context to the existing systemd process.



--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Casey Schaufler Aug. 20, 2021, 11:48 p.m. UTC | #15
On 8/20/2021 12:17 PM, Casey Schaufler wrote:
> On 8/20/2021 12:06 PM, Paul Moore wrote:
>> On Thu, Aug 19, 2021 at 6:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 8/18/2021 5:56 PM, Casey Schaufler wrote:
>>>> On 8/18/2021 5:47 PM, Paul Moore wrote:
>>>>> ...
>>>>> I just spent a few minutes tracing the code paths up from audit
>>>>> through netlink and then through the socket layer and I'm not seeing
>>>>> anything obvious where the path differs from any other syscall;
>>>>> current->audit_context *should* be valid just like any other syscall.
>>>>> However, I do have to ask, are you only seeing these audit records
>>>>> with a current->audit_context equal to NULL during early boot?
>>>> Nope. Sorry.
>>> It looks as if all of the NULL audit_context cases are for either
>>> auditd or systemd. Given what the events are, this isn't especially
>>> surprising.
>> I think we may be back to the "early boot" theory.
>>
>> Unless you explicitly enable audit on the kernel cmdline, e.g.
>> "audit=1", processes started before userspace enables audit will not
>> have a properly allocated audit_context; see the "if
>> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
>> the reason why.

I found a hack-around that no one will like. I changed that check to be

(likely(!audit_ever_enabled) && !lsm_multiple_contexts())

It probably introduces a memory leak and/or performance degradation,
but it has the desired affect.


>>
>> I could be wrong here, but I suspect if you add "audit=1" to your
>> kernel command line those remaining cases of NULL audit_contexts will
>> resolve themselves.  If not, we still have work to do ... well, I mean
>> we still have (different) work to do even if this solves the mystery,
>> it's just that we can now explain what you are seeing :)
> Yup, adding "audit=1" to the command line appears to have gotten
> systemd an audit context. It looks like user space enabling audit
> doesn't assign an audit context to the existing systemd process.
>


--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 24, 2021, 2:45 p.m. UTC | #16
On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 8/20/2021 12:06 PM, Paul Moore wrote:
> >> Unless you explicitly enable audit on the kernel cmdline, e.g.
> >> "audit=1", processes started before userspace enables audit will not
> >> have a properly allocated audit_context; see the "if
> >> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
> >> the reason why.
>
> I found a hack-around that no one will like. I changed that check to be
>
> (likely(!audit_ever_enabled) && !lsm_multiple_contexts())
>
> It probably introduces a memory leak and/or performance degradation,
> but it has the desired affect.

I can't speak for everyone, but I know I don't like that as a solution
;)  I imagine such a change would also draw the ire of the never-audit
crowd and the distros themselves.  However, I understand the need to
just get *something* in place so you can continue to test/develop;
it's fine to keep that for now, but I'm going to be very disappointed
if that line finds its way into the next posted patchset revision.

I'm very much open to ideas but my gut feeling is that the end
solution is going to be changes to audit_log_start() and
audit_log_end().  In my mind the primary reason for this hunch is that
support for multiple LSMs[*] needs to be transparent to the various
callers in the kernel; this means the existing audit pattern of ...

  audit_log_start(...);
  audit_log_format(...);
  audit_log_end(...);

... should be preserved and be unchanged from what it is now.  We've
already talked in some general terms about what such changes might
look like, but to summarize the previous discussions, I think we would
need to think about the following things:

* Adding a timestamp/serial field to the audit_buffer struct, it could
even be in a union with the audit_context pointer as we would never
need both at the same time: if the audit_context ptr is non-NULL you
use that, otherwise you use the timestamp.  The audit_buffer timestamp
would not eliminate the need for the timestamp info in the
audit_context struct for what I hope are obvious reasons.

* Additional logic in audit_log_end() to generate additional ancillary
records for LSM labels.  This will likely mean storing the message
"type" passed to audit_log_start() in the audit_record struct and
using that information in audit_log_end() to decide if a LSM ancillary
record is needed.  Logistically this would likely mean converting the
existing audit_log_end() function into a static/private
__audit_log_end() and converting audit_log_end() into something like
this:

  void audit_log_end(ab)
  {
    __audit_log_end(ab); // rm the ab cleanup from __audit_log_end
    if (ab->anc_mlsm) {
      // generate the multiple lsm record
    }
    audit_buffer_free(ab);
  }

* Something else that I'm surely forgetting :)

At the end of all this we may find that the "local" audit_context
concept is no longer needed.  It was originally created to deal with
grouping ancillary records with non-syscall records into a single
event; while what we are talking about above is different, I believe
that our likely solution will also work to solve the original "local"
audit_context use case as well.  We'll have to see how this goes.

[*] I expect that the audit container ID work will have similar issues
and need a similar solution, I'm surprised it hasn't come up yet.
Casey Schaufler Aug. 24, 2021, 3:20 p.m. UTC | #17
On 8/24/2021 7:45 AM, Paul Moore wrote:
> On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 8/20/2021 12:06 PM, Paul Moore wrote:
>>>> Unless you explicitly enable audit on the kernel cmdline, e.g.
>>>> "audit=1", processes started before userspace enables audit will not
>>>> have a properly allocated audit_context; see the "if
>>>> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
>>>> the reason why.
>> I found a hack-around that no one will like. I changed that check to be
>>
>> (likely(!audit_ever_enabled) && !lsm_multiple_contexts())
>>
>> It probably introduces a memory leak and/or performance degradation,
>> but it has the desired affect.
> I can't speak for everyone, but I know I don't like that as a solution
> ;)  I imagine such a change would also draw the ire of the never-audit
> crowd and the distros themselves.  However, I understand the need to
> just get *something* in place so you can continue to test/develop;
> it's fine to keep that for now, but I'm going to be very disappointed
> if that line finds its way into the next posted patchset revision.

As I said, it's a hack-around that demonstrates the scope of the
problem. Had you expressed enthusiastic approval for it I'd have
been very surprised.

> I'm very much open to ideas but my gut feeling is that the end
> solution is going to be changes to audit_log_start() and
> audit_log_end().  In my mind the primary reason for this hunch is that
> support for multiple LSMs[*] needs to be transparent to the various
> callers in the kernel; this means the existing audit pattern of ...
>
>   audit_log_start(...);
>   audit_log_format(...);
>   audit_log_end(...);
>
> ... should be preserved and be unchanged from what it is now.  We've
> already talked in some general terms about what such changes might
> look like, but to summarize the previous discussions, I think we would
> need to think about the following things:

I will give this a shot.

>
> * Adding a timestamp/serial field to the audit_buffer struct, it could
> even be in a union with the audit_context pointer as we would never
> need both at the same time: if the audit_context ptr is non-NULL you
> use that, otherwise you use the timestamp.  The audit_buffer timestamp
> would not eliminate the need for the timestamp info in the
> audit_context struct for what I hope are obvious reasons.
>
> * Additional logic in audit_log_end() to generate additional ancillary
> records for LSM labels.  This will likely mean storing the message
> "type" passed to audit_log_start() in the audit_record struct and
> using that information in audit_log_end() to decide if a LSM ancillary
> record is needed.  Logistically this would likely mean converting the
> existing audit_log_end() function into a static/private
> __audit_log_end() and converting audit_log_end() into something like
> this:
>
>   void audit_log_end(ab)
>   {
>     __audit_log_end(ab); // rm the ab cleanup from __audit_log_end
>     if (ab->anc_mlsm) {
>       // generate the multiple lsm record
>     }
>     audit_buffer_free(ab);
>   }
>
> * Something else that I'm surely forgetting :)
>
> At the end of all this we may find that the "local" audit_context
> concept is no longer needed.  It was originally created to deal with
> grouping ancillary records with non-syscall records into a single
> event; while what we are talking about above is different, I believe
> that our likely solution will also work to solve the original "local"
> audit_context use case as well.  We'll have to see how this goes.

I'll keep that in mind as I fiddle about.

> [*] I expect that the audit container ID work will have similar issues
> and need a similar solution, I'm surprised it hasn't come up yet.

Hmm. That effort has been quiet lately. Too quiet.



--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Paul Moore Aug. 24, 2021, 4:14 p.m. UTC | #18
On Tue, Aug 24, 2021 at 11:20 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/24/2021 7:45 AM, Paul Moore wrote:
> > On Fri, Aug 20, 2021 at 7:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 8/20/2021 12:06 PM, Paul Moore wrote:
> >>>> Unless you explicitly enable audit on the kernel cmdline, e.g.
> >>>> "audit=1", processes started before userspace enables audit will not
> >>>> have a properly allocated audit_context; see the "if
> >>>> (likely(!audit_ever_enabled))" check at the top of audit_alloc() for
> >>>> the reason why.
> >> I found a hack-around that no one will like. I changed that check to be
> >>
> >> (likely(!audit_ever_enabled) && !lsm_multiple_contexts())
> >>
> >> It probably introduces a memory leak and/or performance degradation,
> >> but it has the desired affect.
> > I can't speak for everyone, but I know I don't like that as a solution
> > ;)  I imagine such a change would also draw the ire of the never-audit
> > crowd and the distros themselves.  However, I understand the need to
> > just get *something* in place so you can continue to test/develop;
> > it's fine to keep that for now, but I'm going to be very disappointed
> > if that line finds its way into the next posted patchset revision.
>
> As I said, it's a hack-around that demonstrates the scope of the
> problem. Had you expressed enthusiastic approval for it I'd have
> been very surprised.

That's okay, you can admit you were trying to catch me not paying attention ;)

> > I'm very much open to ideas but my gut feeling is that the end
> > solution is going to be changes to audit_log_start() and
> > audit_log_end().  In my mind the primary reason for this hunch is that
> > support for multiple LSMs[*] needs to be transparent to the various
> > callers in the kernel; this means the existing audit pattern of ...
> >
> >   audit_log_start(...);
> >   audit_log_format(...);
> >   audit_log_end(...);
> >
> > ... should be preserved and be unchanged from what it is now.  We've
> > already talked in some general terms about what such changes might
> > look like, but to summarize the previous discussions, I think we would
> > need to think about the following things:
>
> I will give this a shot.

Thanks.  I'm sure I'm probably missing some detail, but if you get
stuck let me know and I'll try to lend a hand.

> > [*] I expect that the audit container ID work will have similar issues
> > and need a similar solution, I'm surprised it hasn't come up yet.
>
> Hmm. That effort has been quiet lately. Too quiet.

The current delay is intentional and is related to the io_uring work.

When Richard and I first became aware of the io_uring issue Richard
was in the process of readying his latest revision to the audit
container ID patchset and some of the changes he was incorporating, in
my opinion, hinted at the io_uring issue, or at least drew more
attention to that than I was comfortable seeing posted publicly.
Richard discussed this with his management and security response team,
and they felt differently.  I told Richard that I didn't want to block
him posting an update to the patchset, but that I felt it would be The
Wrong Thing To Do and if he did post the patchset I would likely
ignore it until after the io_uring fix had been posted so as to not
draw additional attention to his changes.  I can't speak for Richard's
mindset, but he appeared anxious to post his changes regardless of my
concerns, and he did so shortly afterwards.

That's why you haven't seen much progress on this for a while, and why
you will see me comment on the latest patchset after the io_uring
patches land in -next after the next merge window closes.
diff mbox series

Patch

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 2c3a2348a144..3520caa0260c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2722,7 +2722,7 @@  static void binder_transaction(struct binder_proc *proc,
 		 * case well anyway.
 		 */
 		security_task_getsecid_obj(proc->tsk, &blob);
-		ret = security_secid_to_secctx(&blob, &lsmctx);
+		ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
 		if (ret) {
 			return_error = BR_FAILED_REPLY;
 			return_error_param = ret;
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 97cd7471e572..85eb87f6f92d 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -291,6 +291,7 @@  extern int  audit_alloc(struct task_struct *task);
 extern void __audit_free(struct task_struct *task);
 extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
 extern void audit_free_context(struct audit_context *context);
+extern void audit_free_local(struct audit_context *context);
 extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 				  unsigned long a2, unsigned long a3);
 extern void __audit_syscall_exit(int ret_success, long ret_value);
@@ -386,6 +387,19 @@  static inline void audit_ptrace(struct task_struct *t)
 		__audit_ptrace(t);
 }
 
+static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
+{
+	struct audit_context *context = audit_context();
+
+	if (context)
+		return context;
+
+	if (lsm_multiple_contexts())
+		return audit_alloc_local(gfp);
+
+	return NULL;
+}
+
 				/* Private API (for audit.c only) */
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
@@ -560,6 +574,8 @@  extern int audit_signals;
 }
 static inline void audit_free_context(struct audit_context *context)
 { }
+static inline void audit_free_local(struct audit_context *context)
+{ }
 static inline int audit_alloc(struct task_struct *task)
 {
 	return 0;
diff --git a/include/linux/security.h b/include/linux/security.h
index 3e9743118fb9..b3cf68cf2bd6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -182,6 +182,8 @@  struct lsmblob {
 #define LSMBLOB_INVALID		-1	/* Not a valid LSM slot number */
 #define LSMBLOB_NEEDED		-2	/* Slot requested on initialization */
 #define LSMBLOB_NOT_NEEDED	-3	/* Slot not requested */
+#define LSMBLOB_DISPLAY		-4	/* Use the "display" slot */
+#define LSMBLOB_FIRST		-5	/* Use the default "display" slot */
 
 /**
  * lsmblob_init - initialize an lsmblob structure
@@ -248,6 +250,15 @@  static inline u32 lsmblob_value(const struct lsmblob *blob)
 	return 0;
 }
 
+static inline bool lsm_multiple_contexts(void)
+{
+#ifdef CONFIG_SECURITY
+	return lsm_slot_to_name(1) != NULL;
+#else
+	return false;
+#endif
+}
+
 /* These functions are in security/commoncap.c */
 extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
 		       int cap, unsigned int opts);
@@ -578,7 +589,8 @@  int security_setprocattr(const char *lsm, const char *name, void *value,
 			 size_t size);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
-int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
+int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
+			     int display);
 int security_secctx_to_secid(const char *secdata, u32 seclen,
 			     struct lsmblob *blob);
 void security_release_secctx(struct lsmcontext *cp);
@@ -1433,7 +1445,7 @@  static inline int security_ismaclabel(const char *name)
 }
 
 static inline int security_secid_to_secctx(struct lsmblob *blob,
-					   struct lsmcontext *cp)
+					   struct lsmcontext *cp, int display)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/net/netlabel.h b/include/net/netlabel.h
index 73fc25b4042b..216cb1ffc8f0 100644
--- a/include/net/netlabel.h
+++ b/include/net/netlabel.h
@@ -97,7 +97,7 @@  struct calipso_doi;
 
 /* NetLabel audit information */
 struct netlbl_audit {
-	u32 secid;
+	struct lsmblob lsmdata;
 	kuid_t loginuid;
 	unsigned int sessionid;
 };
diff --git a/include/net/scm.h b/include/net/scm.h
index b77a52f93389..f4d567d4885e 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -101,7 +101,7 @@  static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
 		 * and the infrastructure will know which it is.
 		 */
 		lsmblob_init(&lb, scm->secid);
-		err = security_secid_to_secctx(&lb, &context);
+		err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
 
 		if (!err) {
 			put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index cbff7c2a9724..a10fa01f7bf4 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -660,13 +660,22 @@  struct xfrm_spi_skb_cb {
 #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
 
 #ifdef CONFIG_AUDITSYSCALL
-static inline struct audit_buffer *xfrm_audit_start(const char *op)
+static inline struct audit_buffer *xfrm_audit_start(const char *op,
+						    struct audit_context **lac)
 {
+	struct audit_context *context;
 	struct audit_buffer *audit_buf = NULL;
 
 	if (audit_enabled == AUDIT_OFF)
 		return NULL;
-	audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
+	context = audit_context();
+	if (lac != NULL) {
+		if (lsm_multiple_contexts() && context == NULL)
+			context = audit_alloc_local(GFP_ATOMIC);
+		*lac = context;
+	}
+
+	audit_buf = audit_log_start(context, GFP_ATOMIC,
 				    AUDIT_MAC_IPSEC_EVENT);
 	if (audit_buf == NULL)
 		return NULL;
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index daa481729e9b..4432a8bed8e0 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -139,6 +139,7 @@ 
 #define AUDIT_MAC_UNLBL_STCDEL	1417	/* NetLabel: del a static label */
 #define AUDIT_MAC_CALIPSO_ADD	1418	/* NetLabel: add CALIPSO DOI entry */
 #define AUDIT_MAC_CALIPSO_DEL	1419	/* NetLabel: del CALIPSO DOI entry */
+#define AUDIT_MAC_TASK_CONTEXTS	1420	/* Multiple LSM contexts */
 
 #define AUDIT_FIRST_KERN_ANOM_MSG   1700
 #define AUDIT_LAST_KERN_ANOM_MSG    1799
diff --git a/kernel/audit.c b/kernel/audit.c
index 841123390d41..cba63789a164 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -386,10 +386,12 @@  void audit_log_lost(const char *message)
 static int audit_log_config_change(char *function_name, u32 new, u32 old,
 				   int allow_changes)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 	int rc = 0;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (unlikely(!ab))
 		return rc;
 	audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
@@ -399,6 +401,7 @@  static int audit_log_config_change(char *function_name, u32 new, u32 old,
 		allow_changes = 0; /* Something weird, deny request */
 	audit_log_format(ab, " res=%d", allow_changes);
 	audit_log_end(ab);
+	audit_free_local(context);
 	return rc;
 }
 
@@ -1072,12 +1075,6 @@  static void audit_log_common_recv_msg(struct audit_context *context,
 	audit_log_task_context(*ab);
 }
 
-static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
-					   u16 msg_type)
-{
-	audit_log_common_recv_msg(NULL, ab, msg_type);
-}
-
 int is_audit_feature_set(int i)
 {
 	return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1190,6 +1187,7 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct audit_buffer	*ab;
 	u16			msg_type = nlh->nlmsg_type;
 	struct audit_sig_info   *sig_data;
+	struct audit_context	*lcontext;
 
 	err = audit_netlink_ok(skb, msg_type);
 	if (err)
@@ -1357,7 +1355,8 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				if (err)
 					break;
 			}
-			audit_log_user_recv_msg(&ab, msg_type);
+			lcontext = audit_alloc_for_lsm(GFP_KERNEL);
+			audit_log_common_recv_msg(lcontext, &ab, msg_type);
 			if (msg_type != AUDIT_USER_TTY) {
 				/* ensure NULL termination */
 				str[data_len - 1] = '\0';
@@ -1371,6 +1370,7 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				audit_log_n_untrustedstring(ab, str, data_len);
 			}
 			audit_log_end(ab);
+			audit_free_local(lcontext);
 		}
 		break;
 	case AUDIT_ADD_RULE:
@@ -1378,13 +1378,15 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		if (data_len < sizeof(struct audit_rule_data))
 			return -EINVAL;
 		if (audit_enabled == AUDIT_LOCKED) {
-			audit_log_common_recv_msg(audit_context(), &ab,
+			lcontext = audit_alloc_for_lsm(GFP_KERNEL);
+			audit_log_common_recv_msg(lcontext, &ab,
 						  AUDIT_CONFIG_CHANGE);
 			audit_log_format(ab, " op=%s audit_enabled=%d res=0",
 					 msg_type == AUDIT_ADD_RULE ?
 						"add_rule" : "remove_rule",
 					 audit_enabled);
 			audit_log_end(ab);
+			audit_free_local(lcontext);
 			return -EPERM;
 		}
 		err = audit_rule_change(msg_type, seq, data, data_len);
@@ -1394,10 +1396,11 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		break;
 	case AUDIT_TRIM:
 		audit_trim_trees();
-		audit_log_common_recv_msg(audit_context(), &ab,
-					  AUDIT_CONFIG_CHANGE);
+		lcontext = audit_alloc_for_lsm(GFP_KERNEL);
+		audit_log_common_recv_msg(lcontext, &ab, AUDIT_CONFIG_CHANGE);
 		audit_log_format(ab, " op=trim res=1");
 		audit_log_end(ab);
+		audit_free_local(lcontext);
 		break;
 	case AUDIT_MAKE_EQUIV: {
 		void *bufp = data;
@@ -1425,14 +1428,15 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		/* OK, here comes... */
 		err = audit_tag_tree(old, new);
 
-		audit_log_common_recv_msg(audit_context(), &ab,
-					  AUDIT_CONFIG_CHANGE);
+		lcontext = audit_alloc_for_lsm(GFP_KERNEL);
+		audit_log_common_recv_msg(lcontext, &ab, AUDIT_CONFIG_CHANGE);
 		audit_log_format(ab, " op=make_equiv old=");
 		audit_log_untrustedstring(ab, old);
 		audit_log_format(ab, " new=");
 		audit_log_untrustedstring(ab, new);
 		audit_log_format(ab, " res=%d", !err);
 		audit_log_end(ab);
+		audit_free_local(lcontext);
 		kfree(old);
 		kfree(new);
 		break;
@@ -1443,7 +1447,7 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 		if (lsmblob_is_set(&audit_sig_lsm)) {
 			err = security_secid_to_secctx(&audit_sig_lsm,
-						       &context);
+						       &context, LSMBLOB_FIRST);
 			if (err)
 				return err;
 		}
@@ -1498,13 +1502,14 @@  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		old.enabled = t & AUDIT_TTY_ENABLE;
 		old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
 
-		audit_log_common_recv_msg(audit_context(), &ab,
-					  AUDIT_CONFIG_CHANGE);
+		lcontext = audit_alloc_for_lsm(GFP_KERNEL);
+		audit_log_common_recv_msg(lcontext, &ab, AUDIT_CONFIG_CHANGE);
 		audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
 				 " old-log_passwd=%d new-log_passwd=%d res=%d",
 				 old.enabled, s.enabled, old.log_passwd,
 				 s.log_passwd, !err);
 		audit_log_end(ab);
+		audit_free_local(lcontext);
 		break;
 	}
 	default:
@@ -1550,6 +1555,7 @@  static void audit_receive(struct sk_buff  *skb)
 /* Log information about who is connecting to the audit multicast socket */
 static void audit_log_multicast(int group, const char *op, int err)
 {
+	struct audit_context *context;
 	const struct cred *cred;
 	struct tty_struct *tty;
 	char comm[sizeof(current->comm)];
@@ -1558,7 +1564,8 @@  static void audit_log_multicast(int group, const char *op, int err)
 	if (!audit_enabled)
 		return;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_EVENT_LISTENER);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_EVENT_LISTENER);
 	if (!ab)
 		return;
 
@@ -1577,6 +1584,7 @@  static void audit_log_multicast(int group, const char *op, int err)
 	audit_log_d_path_exe(ab, current->mm); /* exe= */
 	audit_log_format(ab, " nl-mcgrp=%d op=%s res=%d", group, op, !err);
 	audit_log_end(ab);
+	audit_free_local(context);
 }
 
 /* Run custom bind function on netlink socket group connect or bind requests. */
@@ -2128,6 +2136,36 @@  void audit_log_key(struct audit_buffer *ab, char *key)
 		audit_log_format(ab, "(null)");
 }
 
+static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)
+{
+	struct audit_buffer *ab;
+	struct lsmcontext lsmdata;
+	bool sep = false;
+	int error;
+	int i;
+
+	ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
+	if (!ab)
+		return; /* audit_panic or being filtered */
+
+	for (i = 0; i < LSMBLOB_ENTRIES; i++) {
+		if (blob->secid[i] == 0)
+			continue;
+		error = security_secid_to_secctx(blob, &lsmdata, i);
+		if (error && error != -EINVAL) {
+			audit_panic("error in audit_log_lsm");
+			return;
+		}
+
+		audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
+				 lsm_slot_to_name(i), lsmdata.context);
+		sep = true;
+
+		security_release_secctx(&lsmdata);
+	}
+	audit_log_end(ab);
+}
+
 int audit_log_task_context(struct audit_buffer *ab)
 {
 	int error;
@@ -2138,7 +2176,18 @@  int audit_log_task_context(struct audit_buffer *ab)
 	if (!lsmblob_is_set(&blob))
 		return 0;
 
-	error = security_secid_to_secctx(&blob, &context);
+	/*
+	 * If there is more than one security module that has a
+	 * subject "context" it's necessary to put the subject data
+	 * into a separate record to maintain compatibility.
+	 */
+	if (lsm_multiple_contexts()) {
+		audit_log_format(ab, " subj=?");
+		audit_log_lsm(ab->ctx, &blob);
+		return 0;
+	}
+
+	error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
 	if (error) {
 		if (error != -EINVAL)
 			goto error_path;
@@ -2274,6 +2323,7 @@  static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 				   unsigned int oldsessionid,
 				   unsigned int sessionid, int rc)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 	uid_t uid, oldloginuid, loginuid;
 	struct tty_struct *tty;
@@ -2281,7 +2331,8 @@  static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 	if (!audit_enabled)
 		return;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_LOGIN);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_LOGIN);
 	if (!ab)
 		return;
 
@@ -2297,6 +2348,7 @@  static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
 			 oldsessionid, sessionid, !rc);
 	audit_put_tty(tty);
 	audit_log_end(ab);
+	audit_free_local(context);
 }
 
 /**
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 1ba14a7a38f7..fd71c6bac200 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1098,12 +1098,14 @@  static void audit_list_rules(int seq, struct sk_buff_head *q)
 /* Log rule additions and removals */
 static void audit_log_rule_change(char *action, struct audit_krule *rule, int res)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 
 	if (!audit_enabled)
 		return;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (!ab)
 		return;
 	audit_log_session_info(ab);
@@ -1112,6 +1114,7 @@  static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
 	audit_log_key(ab, rule->filterkey);
 	audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
 	audit_log_end(ab);
+	audit_free_local(context);
 }
 
 /**
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 0e58a3ab56f5..01fdcbf468c0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -993,12 +993,11 @@  struct audit_context *audit_alloc_local(gfp_t gfpflags)
 	context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
 	if (!context) {
 		audit_log_lost("out of memory in audit_alloc_local");
-		goto out;
+		return NULL;
 	}
 	context->serial = audit_serial();
 	ktime_get_coarse_real_ts64(&context->ctime);
 	context->local = true;
-out:
 	return context;
 }
 EXPORT_SYMBOL(audit_alloc_local);
@@ -1019,6 +1018,13 @@  void audit_free_context(struct audit_context *context)
 }
 EXPORT_SYMBOL(audit_free_context);
 
+void audit_free_local(struct audit_context *context)
+{
+	if (context && context->local)
+		audit_free_context(context);
+}
+EXPORT_SYMBOL(audit_free_local);
+
 static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 				 kuid_t auid, kuid_t uid,
 				 unsigned int sessionid,
@@ -1036,7 +1042,7 @@  static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 			 from_kuid(&init_user_ns, auid),
 			 from_kuid(&init_user_ns, uid), sessionid);
 	if (lsmblob_is_set(blob)) {
-		if (security_secid_to_secctx(blob, &lsmctx)) {
+		if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
 			audit_log_format(ab, " obj=(none)");
 			rc = 1;
 		} else {
@@ -1282,7 +1288,8 @@  static void show_special(struct audit_context *context, int *call_panic)
 			struct lsmblob blob;
 
 			lsmblob_init(&blob, osid);
-			if (security_secid_to_secctx(&blob, &lsmcxt)) {
+			if (security_secid_to_secctx(&blob, &lsmcxt,
+						     LSMBLOB_FIRST)) {
 				audit_log_format(ab, " osid=%u", osid);
 				*call_panic = 1;
 			} else {
@@ -1439,7 +1446,7 @@  static void audit_log_name(struct audit_context *context, struct audit_names *n,
 		struct lsmcontext lsmctx;
 
 		lsmblob_init(&blob, n->osid);
-		if (security_secid_to_secctx(&blob, &lsmctx)) {
+		if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) {
 			audit_log_format(ab, " osid=%u", n->osid);
 			if (call_panic)
 				*call_panic = 2;
@@ -2637,10 +2644,12 @@  void __audit_ntp_log(const struct audit_ntp_data *ad)
 void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
 		       enum audit_nfcfgop op, gfp_t gfp)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 	char comm[sizeof(current->comm)];
 
-	ab = audit_log_start(audit_context(), gfp, AUDIT_NETFILTER_CFG);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, gfp, AUDIT_NETFILTER_CFG);
 	if (!ab)
 		return;
 	audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
@@ -2651,6 +2660,7 @@  void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
 	audit_log_format(ab, " comm=");
 	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 	audit_log_end(ab);
+	audit_free_local(context);
 }
 EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
 
@@ -2685,6 +2695,7 @@  static void audit_log_task(struct audit_buffer *ab)
  */
 void audit_core_dumps(long signr)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 
 	if (!audit_enabled)
@@ -2693,12 +2704,14 @@  void audit_core_dumps(long signr)
 	if (signr == SIGQUIT)	/* don't care for those */
 		return;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_ANOM_ABEND);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
 	if (unlikely(!ab))
 		return;
 	audit_log_task(ab);
 	audit_log_format(ab, " sig=%ld res=1", signr);
 	audit_log_end(ab);
+	audit_free_local(context);
 }
 
 /**
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index ae073b642fa7..5c0029a3a595 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -140,7 +140,7 @@  static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
 		return;
 
 	lsmblob_init(&lb, secid);
-	err = security_secid_to_secctx(&lb, &context);
+	err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
 	if (err)
 		return;
 
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 668b31ecd638..05bdbb942499 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -347,7 +347,7 @@  static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
 	 * security_secid_to_secctx() will know which security module
 	 * to use to create the secctx.  */
 	lsmblob_init(&blob, ct->secmark);
-	ret = security_secid_to_secctx(&blob, &context);
+	ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
 	if (ret)
 		return 0;
 
@@ -658,7 +658,7 @@  static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
 	struct lsmblob blob;
 	struct lsmcontext context;
 
-	ret = security_secid_to_secctx(&blob, &context);
+	ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
 	if (ret)
 		return 0;
 
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index b5796a8e5e90..3da3770e9739 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -177,7 +177,7 @@  static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
 	struct lsmcontext context;
 
 	lsmblob_init(&blob, ct->secmark);
-	ret = security_secid_to_secctx(&blob, &context);
+	ret = security_secid_to_secctx(&blob, &context, LSMBLOB_DISPLAY);
 	if (ret)
 		return;
 
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index cffb04baf7b8..9bef0bddf7d6 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -316,7 +316,7 @@  static void nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsmcontext *context)
 		 * blob. security_secid_to_secctx() will know which security
 		 * module to use to create the secctx.  */
 		lsmblob_init(&blob, skb->secmark);
-		security_secid_to_secctx(&blob, context);
+		security_secid_to_secctx(&blob, context, LSMBLOB_DISPLAY);
 	}
 
 	read_unlock_bh(&skb->sk->sk_callback_lock);
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 289602835b75..245f63f5773a 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -437,7 +437,8 @@  int netlbl_unlhsh_add(struct net *net,
 unlhsh_add_return:
 	rcu_read_unlock();
 	if (audit_buf != NULL) {
-		if (security_secid_to_secctx(lsmblob, &context) == 0) {
+		if (security_secid_to_secctx(lsmblob, &context,
+					     LSMBLOB_FIRST) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s",
 					 context.context);
 			security_release_secctx(&context);
@@ -492,7 +493,8 @@  static int netlbl_unlhsh_remove_addr4(struct net *net,
 		if (dev != NULL)
 			dev_put(dev);
 		if (entry != NULL &&
-		    security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
+		    security_secid_to_secctx(&entry->lsmblob, &context,
+					     LSMBLOB_FIRST) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s",
 					 context.context);
 			security_release_secctx(&context);
@@ -552,7 +554,8 @@  static int netlbl_unlhsh_remove_addr6(struct net *net,
 		if (dev != NULL)
 			dev_put(dev);
 		if (entry != NULL &&
-		    security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
+		    security_secid_to_secctx(&entry->lsmblob, &context,
+					     LSMBLOB_FIRST) == 0) {
 			audit_log_format(audit_buf, " sec_obj=%s",
 					 context.context);
 			security_release_secctx(&context);
@@ -738,11 +741,10 @@  static void netlbl_unlabel_acceptflg_set(u8 value,
 	netlabel_unlabel_acceptflg = value;
 	audit_buf = netlbl_audit_start_common(AUDIT_MAC_UNLBL_ALLOW,
 					      audit_info);
-	if (audit_buf != NULL) {
+	if (audit_buf != NULL)
 		audit_log_format(audit_buf,
 				 " unlbl_accept=%u old=%u", value, old_val);
-		audit_log_end(audit_buf);
-	}
+	audit_log_end(audit_buf);
 }
 
 /**
@@ -1122,7 +1124,7 @@  static int netlbl_unlabel_staticlist_gen(u32 cmd,
 		lsmb = (struct lsmblob *)&addr6->lsmblob;
 	}
 
-	ret_val = security_secid_to_secctx(lsmb, &context);
+	ret_val = security_secid_to_secctx(lsmb, &context, LSMBLOB_FIRST);
 	if (ret_val != 0)
 		goto list_cb_failure;
 	ret_val = nla_put(cb_arg->skb,
@@ -1528,14 +1530,11 @@  int __init netlbl_unlabel_defconf(void)
 	int ret_val;
 	struct netlbl_dom_map *entry;
 	struct netlbl_audit audit_info;
-	struct lsmblob blob;
 
 	/* Only the kernel is allowed to call this function and the only time
 	 * it is called is at bootup before the audit subsystem is reporting
 	 * messages so don't worry to much about these values. */
-	security_task_getsecid_subj(current, &blob);
-	/* scaffolding until audit_info.secid is converted */
-	audit_info.secid = blob.secid[0];
+	security_task_getsecid_subj(current, &audit_info.lsmdata);
 	audit_info.loginuid = GLOBAL_ROOT_UID;
 	audit_info.sessionid = 0;
 
diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
index 951ba0639d20..9c43c3cb2088 100644
--- a/net/netlabel/netlabel_user.c
+++ b/net/netlabel/netlabel_user.c
@@ -85,7 +85,6 @@  struct audit_buffer *netlbl_audit_start_common(int type,
 {
 	struct audit_buffer *audit_buf;
 	struct lsmcontext context;
-	struct lsmblob blob;
 
 	if (audit_enabled == AUDIT_OFF)
 		return NULL;
@@ -98,11 +97,14 @@  struct audit_buffer *netlbl_audit_start_common(int type,
 			 from_kuid(&init_user_ns, audit_info->loginuid),
 			 audit_info->sessionid);
 
-	lsmblob_init(&blob, audit_info->secid);
-	if (audit_info->secid != 0 &&
-	    security_secid_to_secctx(&blob, &context) == 0) {
-		audit_log_format(audit_buf, " subj=%s", context.context);
-		security_release_secctx(&context);
+	if (lsmblob_is_set(&audit_info->lsmdata)) {
+		if (!lsm_multiple_contexts() &&
+		    security_secid_to_secctx(&audit_info->lsmdata, &context,
+					     LSMBLOB_FIRST) == 0) {
+			audit_log_format(audit_buf, " subj=%s",
+					 context.context);
+			security_release_secctx(&context);
+		}
 	}
 
 	return audit_buf;
diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h
index aa31f7bf79ee..e5b15ad41df7 100644
--- a/net/netlabel/netlabel_user.h
+++ b/net/netlabel/netlabel_user.h
@@ -32,11 +32,7 @@ 
  */
 static inline void netlbl_netlink_auditinfo(struct netlbl_audit *audit_info)
 {
-	struct lsmblob blob;
-
-	security_task_getsecid_subj(current, &blob);
-	/* scaffolding until secid is converted */
-	audit_info->secid = blob.secid[0];
+	security_task_getsecid_subj(current, &audit_info->lsmdata);
 	audit_info->loginuid = audit_get_loginuid(current);
 	audit_info->sessionid = audit_get_sessionid(current);
 }
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 827d84255021..2152e319951d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4178,30 +4178,34 @@  static void xfrm_audit_common_policyinfo(struct xfrm_policy *xp,
 
 void xfrm_audit_policy_add(struct xfrm_policy *xp, int result, bool task_valid)
 {
+	struct audit_context *context;
 	struct audit_buffer *audit_buf;
 
-	audit_buf = xfrm_audit_start("SPD-add");
+	audit_buf = xfrm_audit_start("SPD-add", &context);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_usrinfo(task_valid, audit_buf);
 	audit_log_format(audit_buf, " res=%u", result);
 	xfrm_audit_common_policyinfo(xp, audit_buf);
 	audit_log_end(audit_buf);
+	audit_free_local(context);
 }
 EXPORT_SYMBOL_GPL(xfrm_audit_policy_add);
 
 void xfrm_audit_policy_delete(struct xfrm_policy *xp, int result,
 			      bool task_valid)
 {
+	struct audit_context *context;
 	struct audit_buffer *audit_buf;
 
-	audit_buf = xfrm_audit_start("SPD-delete");
+	audit_buf = xfrm_audit_start("SPD-delete", &context);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_usrinfo(task_valid, audit_buf);
 	audit_log_format(audit_buf, " res=%u", result);
 	xfrm_audit_common_policyinfo(xp, audit_buf);
 	audit_log_end(audit_buf);
+	audit_free_local(context);
 }
 EXPORT_SYMBOL_GPL(xfrm_audit_policy_delete);
 #endif
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a2f4001221d1..4d174f42eb60 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2796,29 +2796,33 @@  static void xfrm_audit_helper_pktinfo(struct sk_buff *skb, u16 family,
 
 void xfrm_audit_state_add(struct xfrm_state *x, int result, bool task_valid)
 {
+	struct audit_context *context;
 	struct audit_buffer *audit_buf;
 
-	audit_buf = xfrm_audit_start("SAD-add");
+	audit_buf = xfrm_audit_start("SAD-add", &context);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_usrinfo(task_valid, audit_buf);
 	xfrm_audit_helper_sainfo(x, audit_buf);
 	audit_log_format(audit_buf, " res=%u", result);
 	audit_log_end(audit_buf);
+	audit_free_local(context);
 }
 EXPORT_SYMBOL_GPL(xfrm_audit_state_add);
 
 void xfrm_audit_state_delete(struct xfrm_state *x, int result, bool task_valid)
 {
+	struct audit_context *context;
 	struct audit_buffer *audit_buf;
 
-	audit_buf = xfrm_audit_start("SAD-delete");
+	audit_buf = xfrm_audit_start("SAD-delete", &context);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_usrinfo(task_valid, audit_buf);
 	xfrm_audit_helper_sainfo(x, audit_buf);
 	audit_log_format(audit_buf, " res=%u", result);
 	audit_log_end(audit_buf);
+	audit_free_local(context);
 }
 EXPORT_SYMBOL_GPL(xfrm_audit_state_delete);
 
@@ -2828,7 +2832,7 @@  void xfrm_audit_state_replay_overflow(struct xfrm_state *x,
 	struct audit_buffer *audit_buf;
 	u32 spi;
 
-	audit_buf = xfrm_audit_start("SA-replay-overflow");
+	audit_buf = xfrm_audit_start("SA-replay-overflow", NULL);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_pktinfo(skb, x->props.family, audit_buf);
@@ -2846,7 +2850,7 @@  void xfrm_audit_state_replay(struct xfrm_state *x,
 	struct audit_buffer *audit_buf;
 	u32 spi;
 
-	audit_buf = xfrm_audit_start("SA-replayed-pkt");
+	audit_buf = xfrm_audit_start("SA-replayed-pkt", NULL);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_pktinfo(skb, x->props.family, audit_buf);
@@ -2861,7 +2865,7 @@  void xfrm_audit_state_notfound_simple(struct sk_buff *skb, u16 family)
 {
 	struct audit_buffer *audit_buf;
 
-	audit_buf = xfrm_audit_start("SA-notfound");
+	audit_buf = xfrm_audit_start("SA-notfound", NULL);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_pktinfo(skb, family, audit_buf);
@@ -2875,7 +2879,7 @@  void xfrm_audit_state_notfound(struct sk_buff *skb, u16 family,
 	struct audit_buffer *audit_buf;
 	u32 spi;
 
-	audit_buf = xfrm_audit_start("SA-notfound");
+	audit_buf = xfrm_audit_start("SA-notfound", NULL);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_pktinfo(skb, family, audit_buf);
@@ -2893,7 +2897,7 @@  void xfrm_audit_state_icvfail(struct xfrm_state *x,
 	__be32 net_spi;
 	__be32 net_seq;
 
-	audit_buf = xfrm_audit_start("SA-icv-failure");
+	audit_buf = xfrm_audit_start("SA-icv-failure", NULL);
 	if (audit_buf == NULL)
 		return;
 	xfrm_audit_helper_pktinfo(skb, x->props.family, audit_buf);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 691f68d478f1..3481990a25a6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -342,6 +342,7 @@  void ima_store_measurement(struct integrity_iint_cache *iint,
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 	char *hash;
 	const char *algo_name = hash_algo_name[iint->ima_hash->algo];
@@ -358,8 +359,8 @@  void ima_audit_measurement(struct integrity_iint_cache *iint,
 		hex_byte_pack(hash + (i * 2), iint->ima_hash->digest[i]);
 	hash[i * 2] = '\0';
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL,
-			     AUDIT_INTEGRITY_RULE);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
 	if (!ab)
 		goto out;
 
@@ -369,6 +370,7 @@  void ima_audit_measurement(struct integrity_iint_cache *iint,
 
 	audit_log_task_info(ab);
 	audit_log_end(ab);
+	audit_free_local(context);
 
 	iint->flags |= IMA_AUDITED;
 out:
diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 29220056207f..c3b313886e15 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -38,13 +38,15 @@  void integrity_audit_message(int audit_msgno, struct inode *inode,
 			     const char *cause, int result, int audit_info,
 			     int errno)
 {
+	struct audit_context *context;
 	struct audit_buffer *ab;
 	char name[TASK_COMM_LEN];
 
 	if (!integrity_audit_info && audit_info == 1)	/* Skip info messages */
 		return;
 
-	ab = audit_log_start(audit_context(), GFP_KERNEL, audit_msgno);
+	context = audit_alloc_for_lsm(GFP_KERNEL);
+	ab = audit_log_start(context, GFP_KERNEL, audit_msgno);
 	audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
 			 task_pid_nr(current),
 			 from_kuid(&init_user_ns, current_uid()),
@@ -64,4 +66,5 @@  void integrity_audit_message(int audit_msgno, struct inode *inode,
 	}
 	audit_log_format(ab, " res=%d errno=%d", !result, errno);
 	audit_log_end(ab);
+	audit_free_local(context);
 }
diff --git a/security/security.c b/security/security.c
index cb359e185d1a..5d7fd982f84a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2309,7 +2309,7 @@  int security_setprocattr(const char *lsm, const char *name, void *value,
 		hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
 				     list) {
 			rc = hp->hook.setprocattr(name, value, size);
-			if (rc < 0)
+			if (rc < 0 && rc != -EINVAL)
 				return rc;
 		}
 
@@ -2354,13 +2354,31 @@  int security_ismaclabel(const char *name)
 }
 EXPORT_SYMBOL(security_ismaclabel);
 
-int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp)
+int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
+			     int ilsm)
 {
 	struct security_hook_list *hp;
-	int ilsm = lsm_task_ilsm(current);
 
 	memset(cp, 0, sizeof(*cp));
 
+	/*
+	 * ilsm either is the slot number use for formatting
+	 * or an instruction on which relative slot to use.
+	 */
+	if (ilsm == LSMBLOB_DISPLAY)
+		ilsm = lsm_task_ilsm(current);
+	else if (ilsm == LSMBLOB_FIRST)
+		ilsm = LSMBLOB_INVALID;
+	else if (ilsm < 0) {
+		WARN_ONCE(true,
+			"LSM: %s unknown interface LSM\n", __func__);
+		ilsm = LSMBLOB_INVALID;
+	} else if (ilsm >= lsm_slot) {
+		WARN_ONCE(true,
+			"LSM: %s invalid interface LSM\n", __func__);
+		ilsm = LSMBLOB_INVALID;
+	}
+
 	hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
 		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
 			continue;
@@ -2390,7 +2408,7 @@  int security_secctx_to_secid(const char *secdata, u32 seclen,
 			return hp->hook.secctx_to_secid(secdata, seclen,
 						&blob->secid[hp->lsmid->slot]);
 	}
-	return 0;
+	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(security_secctx_to_secid);
 
@@ -2884,23 +2902,17 @@  int security_key_getsecurity(struct key *key, char **_buffer)
 int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule)
 {
 	struct security_hook_list *hp;
-	bool one_is_good = false;
-	int rc = 0;
-	int trc;
+	int ilsm = lsm_task_ilsm(current);
 
 	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_init, list) {
 		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
 			continue;
-		trc = hp->hook.audit_rule_init(field, op, rulestr,
-					       &lsmrule[hp->lsmid->slot]);
-		if (trc == 0)
-			one_is_good = true;
-		else
-			rc = trc;
+		if (ilsm != LSMBLOB_INVALID && ilsm != hp->lsmid->slot)
+			continue;
+		return hp->hook.audit_rule_init(field, op, rulestr,
+						&lsmrule[hp->lsmid->slot]);
 	}
-	if (one_is_good)
-		return 0;
-	return rc;
+	return 0;
 }
 
 int security_audit_rule_known(struct audit_krule *krule)
@@ -2932,6 +2944,8 @@  int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
 			continue;
 		if (lsmrule[hp->lsmid->slot] == NULL)
 			continue;
+		if (lsmrule[hp->lsmid->slot] == NULL)
+			continue;
 		rc = hp->hook.audit_rule_match(blob->secid[hp->lsmid->slot],
 					       field, op,
 					       &lsmrule[hp->lsmid->slot]);
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 9cda52f2ec31..2f0f412fc403 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -185,7 +185,8 @@  static void smk_netlabel_audit_set(struct netlbl_audit *nap)
 
 	nap->loginuid = audit_get_loginuid(current);
 	nap->sessionid = audit_get_sessionid(current);
-	nap->secid = skp->smk_secid;
+	lsmblob_init(&nap->lsmdata, 0);
+	nap->lsmdata.secid[smack_lsmid.slot] = skp->smk_secid;
 }
 
 /*