Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201026125227.54520-1-a.nogikh@gmail.com> <20201026125227.54520-2-a.nogikh@gmail.com>
 <c768f42a-1370-5b38-4f89-357744fd9d5a@schaufler-ca.com>
In-Reply-To: <c768f42a-1370-5b38-4f89-357744fd9d5a@schaufler-ca.com>
From:   Aleksandr Nogikh <nogikh@google.com>
Date:   Tue, 27 Oct 2020 20:29:35 +0300
Message-ID: <CANp29Y5w1ZxDShGQHvJ-F0bM_P4WSrvPoDQtPNnBCG38Ee-x-A@mail.gmail.com>
Subject: Re: [RFC PATCH v2 1/2] security: add fault injection capability
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     Aleksandr Nogikh <a.nogikh@gmail.com>, jmorris@namei.org,
        serge@hallyn.com, akinobu.mita@gmail.com,
        Andrey Konovalov <andreyknvl@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Marco Elver <elver@google.com>, glider@google.com,
        keescook@google.com, LKML <linux-kernel@vger.kernel.org>,
        linux-security-module@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

(resending the previous message in a plain/text mode)

On Mon, Oct 26, 2020 at 7:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
[...]
> > -     int RC = IRC;                                           \
> > -     do {                                                    \
> > +     int RC = lsm_hooks_inject_fail();                       \
> > +     if (RC == 0) {                                                          \
>
> Injecting the failure here will prevent the loaded LSM hooks from
> being called.
In this RFC, fault injection was intentionally placed before the code that
invokes LSM hooks. The reasoning was that it would simultaneously check
how the kernel code reacts to LSM denials and the effect of fault injections
on LSM modules.

>
> >               struct security_hook_list *P;                   \
> > +             RC = IRC;                                                               \
> >                                                               \
> >               hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> >                       RC = P->hook.FUNC(__VA_ARGS__);         \
> >                       if (RC != 0)                            \
> >                               break;                          \
> >               }                                               \
> > -     } while (0);                                            \
> > +     }                                                       \
>
> Injecting the failure here would allow the loaded LSM hooks to
> be called. It shouldn't make a difference, but hooks with side-effects
> are always possible. I don't have an issue either way.
>
> >       RC;                                                     \
> >  })
> >
>
Should we expect LSM modules to properly handle the cases when their
hooks with side effects were not invoked (unlike the selinux crash that
is described in the cover letter)? From the source code it seems that a
failure/denial from one module prevents the execution of the subsequent
hooks, so this looks like a realistic scenario.

If that is not true in general and depends on the specific active modules,
then it probably makes sense to introduce an option to control whether to
inject faults at the beginning of call_int_hook() or after the hooks have
been invoked.