Hi
On Mon, Apr 8, 2019 at 6:07 PM Kees Cook <keesc...@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.rheinsb...@gmail.com>
> Link:
> https://lkml.kernel.org/r/cadydso6k8vyb1eryt4g6+ehrlcvb68gabhvwuulkyjczcyn...@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook <keesc...@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.rheinsb...@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
