Re: [PATCH v35 22/29] Audit: Keep multiple LSM data in audit_names

From: Paul Moore <paul@paul-moore.com>
To: John Johansen <john.johansen@canonical.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	linux-audit@redhat.com, keescook@chromium.org,
	penguin-kernel@i-love.sakura.ne.jp,
	stephen.smalley.work@gmail.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v35 22/29] Audit: Keep multiple LSM data in audit_names
Date: Tue, 26 Apr 2022 13:57:55 -0400	[thread overview]
Message-ID: <CAHC9VhRhDGMUH-WfyoMDLdDFWbzTcDGhKFZNB22-Ha3dhUKyCQ@mail.gmail.com> (raw)
In-Reply-To: <c196795a-910a-1e70-4809-c96717767e39@canonical.com>

On Mon, Apr 25, 2022 at 7:32 PM John Johansen
<john.johansen@canonical.com> wrote:
> On 4/18/22 07:59, Casey Schaufler wrote:
> > Replace the osid field in the audit_names structure
> > with a lsmblob structure. This accomodates the use
> > of an lsmblob in security_audit_rule_match() and
> > security_inode_getsecid().
> >
> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > Acked-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  kernel/audit.h   |  2 +-
> >  kernel/auditsc.c | 22 ++++++++--------------
> >  2 files changed, 9 insertions(+), 15 deletions(-)

...

> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 231631f61550..6fe9f2525fc1 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -700,17 +700,16 @@ static int audit_filter_rules(struct task_struct *tsk,
> >                                        * lsmblob, which happens later in
> >                                        * this patch set.
> >                                        */
> > -                                     lsmblob_init(&blob, name->osid);
> >                                       result = security_audit_rule_match(
> > -                                                             &blob,
> > +                                                             &name->lsmblob,
> >                                                               f->type,
> >                                                               f->op,
> >                                                               &f->lsm_rules);
> >                               } else if (ctx) {
> >                                       list_for_each_entry(n, &ctx->names_list, list) {
> > -                                             lsmblob_init(&blob, n->osid);
> >                                               if (security_audit_rule_match(
> > -                                                     &blob, f->type, f->op,
> > +                                                     &n->lsmblob,
> > +                                                     f->type, f->op,
> >                                                       &f->lsm_rules)) {
> >                                                       ++result;
> >                                                       break;
> > @@ -1589,13 +1588,12 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> >                                from_kgid(&init_user_ns, n->gid),
> >                                MAJOR(n->rdev),
> >                                MINOR(n->rdev));
> > -     if (n->osid != 0) {
> > -             struct lsmblob blob;
> > +     if (lsmblob_is_set(&n->lsmblob)) {
> >               struct lsmcontext lsmctx;
> >
> > -             lsmblob_init(&blob, n->osid);
> > -             if (security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_FIRST)) {
> > -                     audit_log_format(ab, " osid=%u", n->osid);
> > +             if (security_secid_to_secctx(&n->lsmblob, &lsmctx,
> > +                                          LSMBLOB_FIRST)) {
> > +                     audit_log_format(ab, " osid=?");
>
> is there something better we can do here? This feels like a regression

Unfortunately no, or at least nothing has been suggested that is an
improvement on this approach.  We could overload the existing field,
but that runs the risk of confusing userspace tooling and potentially
bumping into the buffer limit in some more complex configurations.
The "?" value was chosen as it is a commonly accepted way for the
audit subsystem to indicate that a value is "missing" and in the case
of new/updated userspace tooling this would be an indication to look
for the new record type which provides all of the necessary LSM
labels.  In the case of old/unaware userspace tooling it would serve
as a graceful indicator that something is awry, i.e. you are using new
kernel functionality without updating your userspace.

-- 
paul-moore.com