Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190408160706.GA18786@beast>
In-Reply-To: <20190408160706.GA18786@beast>
From:   David Rheinsberg <david.rheinsberg@gmail.com>
Date:   Tue, 9 Apr 2019 08:21:23 +0200
Message-ID: <CADyDSO4NkEY-SPT-abnyRXT9kGWhF8LbU3XQGNp-5k_15debDg@mail.gmail.com>
Subject: Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
To:     Kees Cook <keescook@chromium.org>
Cc:     James Morris <jmorris@namei.org>,
        John Johansen <john.johansen@canonical.com>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi

On Mon, Apr 8, 2019 at 6:07 PM Kees Cook <keescook@chromium.org> wrote:
>
> Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
> state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
> since it was using the "bool" handler. After being changed to "int",
> this switched to "1" or "0", breaking the userspace AppArmor detection
> of dbus-broker. This restores the Y/N output while keeping the LSM
> infrastructure happy.
>
> Before:
>         $ cat /sys/module/apparmor/parameters/enabled
>         1
>
> After:
>         $ cat /sys/module/apparmor/parameters/enabled
>         Y
>
> Reported-by: David Rheinsberg <david.rheinsberg@gmail.com>
> Link: https://lkml.kernel.org/r/CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This fix, if John is okay with it, is needed in v5.1 to correct the
> userspace regression reported by David.
> ---
>  security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)

This looks good to me. Thanks a lot! If this makes v5.1, I will leave
the apparmor-detection in dbus-broker as it is, unless someone asks me
to parse 0/1 as well?

I cannot judge whether the apparmor_initialized check is correct, but
for the parameter parsing:

Reviewed-by: David Rheinsberg <david.rheinsberg@gmail.com>

Thanks
David

> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..87500bde5a92 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
>  bool aa_g_paranoid_load = true;
>  module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
>
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp);
> +#define param_check_aaintbool param_check_int
> +static const struct kernel_param_ops param_ops_aaintbool = {
> +       .set = param_set_aaintbool,
> +       .get = param_get_aaintbool
> +};
>  /* Boot time disable flag */
>  static int apparmor_enabled __lsm_ro_after_init = 1;
> -module_param_named(enabled, apparmor_enabled, int, 0444);
> +module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>
>  static int __init apparmor_enabled_setup(char *str)
>  {
> @@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct kernel_param *kp)
>         return param_get_uint(buffer, kp);
>  }
>
> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> +{
> +       struct kernel_param kp_local;
> +       bool value;
> +       int error;
> +
> +       if (apparmor_initialized)
> +               return -EPERM;
> +
> +       /* Create local copy, with arg pointing to bool type. */
> +       value = !!*((int *)kp->arg);
> +       memcpy(&kp_local, kp, sizeof(kp_local));
> +       kp_local.arg = &value;
> +
> +       error = param_set_bool(val, &kp_local);
> +       if (!error)
> +               *((int *)kp->arg) = *((bool *)kp_local.arg);
> +       return error;
> +}
> +
> +/*
> + * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
> + * 1/0, this converts the "int that is actually bool" back to bool for
> + * display in the /sys filesystem, while keeping it "int" for the LSM
> + * infrastructure.
> + */
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
> +{
> +       struct kernel_param kp_local;
> +       bool value;
> +
> +       /* Create local copy, with arg pointing to bool type. */
> +       value = !!*((int *)kp->arg);
> +       memcpy(&kp_local, kp, sizeof(kp_local));
> +       kp_local.arg = &value;
> +
> +       return param_get_bool(buffer, &kp_local);
> +}
> +
>  static int param_get_audit(char *buffer, const struct kernel_param *kp)
>  {
>         if (!apparmor_enabled)
> --
> 2.17.1
>
>
> --
> Kees Cook