diff mbox series

[Disco] apparmor: Restore Y/N in /sys for apparmor's "enabled"

Message ID 5f66a2fe-abe9-381f-b337-99b93d8dd896@canonical.com
State New
Headers show
Series [Disco] apparmor: Restore Y/N in /sys for apparmor's "enabled" | expand

Commit Message

John Johansen April 9, 2019, 11:29 a.m. UTC 
The attached patch fixes bug 1823379. It is working its way upstream for the 5.1 kernel, but I don't have an upstream sha1 to reference yet

Comments

Seth Forshee April 9, 2019, 1:03 p.m. UTC | #1 
On Tue, Apr 09, 2019 at 04:29:47AM -0700, John Johansen wrote:
> The attached patch fixes bug 1823379. It is working its way upstream for the 5.1 kernel, but I don't have an upstream sha1 to reference yet

> From 4a9047bfe76e2d36cfe8ea6e4a89200fd4ca616d Mon Sep 17 00:00:00 2001
> From: Kees Cook <keescook@chromium.org>
> Date: Mon, 8 Apr 2019 09:07:06 -0700
> Subject: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
> 
> 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
> 
> BugLink: http://bugs.launchpad.net/bugs/1823379
> 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>
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Tested against the lxc test suite and confirmed that this fixes the bug.

Acked-by: Seth Forshee <seth.forshee@canonical.com>

This should be sauce as it's not upstream yet, but that can be fixed
when applying. Thanks!
Andy Whitcroft April 9, 2019, 1:06 p.m. UTC | #2 
On Tue, Apr 09, 2019 at 04:29:47AM -0700, John Johansen wrote:
> The attached patch fixes bug 1823379. It is working its way upstream for the 5.1 kernel, but I don't have an upstream sha1 to reference yet

> From 4a9047bfe76e2d36cfe8ea6e4a89200fd4ca616d Mon Sep 17 00:00:00 2001
> From: Kees Cook <keescook@chromium.org>
> Date: Mon, 8 Apr 2019 09:07:06 -0700
> Subject: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
> 
> 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
> 
> BugLink: http://bugs.launchpad.net/bugs/1823379
> 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>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 7f4b0154dc39..011c360e51f4 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1384,9 +1384,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)
>  {
> @@ -1461,6 +1468,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)

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Stefan Bader April 9, 2019, 1:10 p.m. UTC | #3 
On 09.04.19 13:29, John Johansen wrote:
> The attached patch fixes bug 1823379. It is working its way upstream for the 5.1 kernel, but I don't have an upstream sha1 to reference yet
> 
> 
A lot of code for a minimal change but at least testable.

Acked-by: Stefan Bader <stefan.bader@canonical.com>
Seth Forshee April 9, 2019, 1:12 p.m. UTC | #4 
On Tue, Apr 09, 2019 at 04:29:47AM -0700, John Johansen wrote:
> The attached patch fixes bug 1823379. It is working its way upstream for the 5.1 kernel, but I don't have an upstream sha1 to reference yet

> From 4a9047bfe76e2d36cfe8ea6e4a89200fd4ca616d Mon Sep 17 00:00:00 2001
> From: Kees Cook <keescook@chromium.org>
> Date: Mon, 8 Apr 2019 09:07:06 -0700
> Subject: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
> 
> 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
> 
> BugLink: http://bugs.launchpad.net/bugs/1823379
> 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>
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Applied as 'UBUNTU: SAUCE: apparmor: Restore Y/N in /sys for apparmor's
"enabled",' thanks!
diff mbox series

Patch

From 4a9047bfe76e2d36cfe8ea6e4a89200fd4ca616d Mon Sep 17 00:00:00 2001
From: Kees Cook <keescook@chromium.org>
Date: Mon, 8 Apr 2019 09:07:06 -0700
Subject: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"

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

BugLink: http://bugs.launchpad.net/bugs/1823379
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>
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7f4b0154dc39..011c360e51f4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1384,9 +1384,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)
 {
@@ -1461,6 +1468,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