[Xenial,Zesty] UBUNTU: SAUCE: fix oops when disabled and module parameters, are accessed

Message ID b8b6a59e-4483-addf-85e6-5d45ea2d6364@canonical.com
State New
Headers show
Series
  • [Xenial,Zesty] UBUNTU: SAUCE: fix oops when disabled and module parameters, are accessed
Related show

Commit Message

John Johansen Sept. 1, 2017, 7:05 a.m.
The virtualization of apparmor module parameters failed to take into
account the parameters being accessed when apparmor is not enabled
in some cases.

It also failed to take into account that policy_admin_capable checks
should not be applied to parameters specified at kernel boot as this
is the callback is used before apparmor is initialized.

BugLink: http://bugs.launchpad.net/bugs/1626984
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/lsm.c | 52 +++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

Comments

Colin King Sept. 5, 2017, 12:13 p.m. | #1
On 01/09/17 08:05, John Johansen wrote:
> The virtualization of apparmor module parameters failed to take into
> account the parameters being accessed when apparmor is not enabled
> in some cases.
> 
> It also failed to take into account that policy_admin_capable checks
> should not be applied to parameters specified at kernel boot as this
> is the callback is used before apparmor is initialized.
> 
> BugLink: http://bugs.launchpad.net/bugs/1626984
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/lsm.c | 52 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 70617e50a0d4..7951c3dc9393 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -41,7 +41,7 @@
>  #include "include/mount.h"
>  
>  /* Flag indicating whether initialization completed */
> -int apparmor_initialized __initdata;
> +int apparmor_initialized;
>  
>  DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>  
> @@ -1409,74 +1409,83 @@ __setup("apparmor=", apparmor_enabled_setup);
>  /* set global flag turning off the ability to load policy */
>  static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> +	if (!apparmor_enabled)
> +		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
>  		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aabool(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aabool(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aauint(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +	int error;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> -	return param_set_uint(val, kp);
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
> +
> +	error = param_set_uint(val, kp);
> +	pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
> +
> +	return error;
>  }
>  
>  static int param_get_aauint(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_uint(buffer, kp);
>  }
>  
>  static int param_get_audit(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
>  }
>  
>  static int param_set_audit(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < AUDIT_MAX_INDEX; i++) {
>  		if (strcmp(val, audit_mode_names[i]) == 0) {
> @@ -1490,10 +1499,10 @@ static int param_set_audit(const char *val, struct kernel_param *kp)
>  
>  static int param_get_mode(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  
>  	return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
>  }
> @@ -1501,12 +1510,13 @@ static int param_get_mode(char *buffer, struct kernel_param *kp)
>  static int param_set_mode(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < APPARMOR_MODE_NAMES_MAX_INDEX; i++) {
>  		if (strcmp(val, aa_profile_mode_names[i]) == 0) {
> 
Acked-by: Colin Ian King <colin.king@canonical.com>
Stefan Bader Sept. 5, 2017, 3:23 p.m. | #2
On 01.09.2017 09:05, John Johansen wrote:
> The virtualization of apparmor module parameters failed to take into
> account the parameters being accessed when apparmor is not enabled
> in some cases.
> 
> It also failed to take into account that policy_admin_capable checks
> should not be applied to parameters specified at kernel boot as this
> is the callback is used before apparmor is initialized.
> 
> BugLink: http://bugs.launchpad.net/bugs/1626984
> Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>

> ---
>  security/apparmor/lsm.c | 52 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 70617e50a0d4..7951c3dc9393 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -41,7 +41,7 @@
>  #include "include/mount.h"
>  
>  /* Flag indicating whether initialization completed */
> -int apparmor_initialized __initdata;
> +int apparmor_initialized;
>  
>  DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>  
> @@ -1409,74 +1409,83 @@ __setup("apparmor=", apparmor_enabled_setup);
>  /* set global flag turning off the ability to load policy */
>  static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> +	if (!apparmor_enabled)
> +		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
>  		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aabool(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aabool(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aauint(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +	int error;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> -	return param_set_uint(val, kp);
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
> +
> +	error = param_set_uint(val, kp);
> +	pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
> +
> +	return error;
>  }
>  
>  static int param_get_aauint(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_uint(buffer, kp);
>  }
>  
>  static int param_get_audit(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
>  }
>  
>  static int param_set_audit(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < AUDIT_MAX_INDEX; i++) {
>  		if (strcmp(val, audit_mode_names[i]) == 0) {
> @@ -1490,10 +1499,10 @@ static int param_set_audit(const char *val, struct kernel_param *kp)
>  
>  static int param_get_mode(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  
>  	return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
>  }
> @@ -1501,12 +1510,13 @@ static int param_get_mode(char *buffer, struct kernel_param *kp)
>  static int param_set_mode(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < APPARMOR_MODE_NAMES_MAX_INDEX; i++) {
>  		if (strcmp(val, aa_profile_mode_names[i]) == 0) {
>
Stefan Bader Sept. 15, 2017, 1:11 p.m. | #3
On 01.09.2017 09:05, John Johansen wrote:
> The virtualization of apparmor module parameters failed to take into
> account the parameters being accessed when apparmor is not enabled
> in some cases.
> 
> It also failed to take into account that policy_admin_capable checks
> should not be applied to parameters specified at kernel boot as this
> is the callback is used before apparmor is initialized.
> 
> BugLink: http://bugs.launchpad.net/bugs/1626984
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/lsm.c | 52 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 70617e50a0d4..7951c3dc9393 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -41,7 +41,7 @@
>  #include "include/mount.h"
>  
>  /* Flag indicating whether initialization completed */
> -int apparmor_initialized __initdata;
> +int apparmor_initialized;
>  
>  DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>  
> @@ -1409,74 +1409,83 @@ __setup("apparmor=", apparmor_enabled_setup);
>  /* set global flag turning off the ability to load policy */
>  static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> +	if (!apparmor_enabled)
> +		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
>  		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aabool(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  	return param_set_bool(val, kp);
>  }
>  
>  static int param_get_aabool(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_bool(buffer, kp);
>  }
>  
>  static int param_set_aauint(const char *val, const struct kernel_param *kp)
>  {
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +	int error;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> -	return param_set_uint(val, kp);
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
> +
> +	error = param_set_uint(val, kp);
> +	pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
> +
> +	return error;
>  }
>  
>  static int param_get_aauint(char *buffer, const struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return param_get_uint(buffer, kp);
>  }
>  
>  static int param_get_audit(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  	return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
>  }
>  
>  static int param_set_audit(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < AUDIT_MAX_INDEX; i++) {
>  		if (strcmp(val, audit_mode_names[i]) == 0) {
> @@ -1490,10 +1499,10 @@ static int param_set_audit(const char *val, struct kernel_param *kp)
>  
>  static int param_get_mode(char *buffer, struct kernel_param *kp)
>  {
> -	if (!policy_view_capable(NULL))
> -		return -EPERM;
>  	if (!apparmor_enabled)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_view_capable(NULL))
> +		return -EPERM;
>  
>  	return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
>  }
> @@ -1501,12 +1510,13 @@ static int param_get_mode(char *buffer, struct kernel_param *kp)
>  static int param_set_mode(const char *val, struct kernel_param *kp)
>  {
>  	int i;
> -	if (!policy_admin_capable(NULL))
> -		return -EPERM;
> +
>  	if (!apparmor_enabled)
>  		return -EINVAL;
>  	if (!val)
>  		return -EINVAL;
> +	if (apparmor_initialized && !policy_admin_capable(NULL))
> +		return -EPERM;
>  
>  	for (i = 0; i < APPARMOR_MODE_NAMES_MAX_INDEX; i++) {
>  		if (strcmp(val, aa_profile_mode_names[i]) == 0) {
> 
This patch is already applied in Zesty (and was even before release) as

UBUNTU: SAUCE: apparmor: fix parameters so that the permission test is bypas
sed at boot

    BugLink: http://bugs.launchpad.net/bugs/1678048

Patch

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 70617e50a0d4..7951c3dc9393 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -41,7 +41,7 @@ 
 #include "include/mount.h"
 
 /* Flag indicating whether initialization completed */
-int apparmor_initialized __initdata;
+int apparmor_initialized;
 
 DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
 
@@ -1409,74 +1409,83 @@  __setup("apparmor=", apparmor_enabled_setup);
 /* set global flag turning off the ability to load policy */
 static int param_set_aalockpolicy(const char *val, const struct kernel_param *kp)
 {
-	if (!policy_admin_capable(NULL))
+	if (!apparmor_enabled)
+		return -EINVAL;
+	if (apparmor_initialized && !policy_admin_capable(NULL))
 		return -EPERM;
 	return param_set_bool(val, kp);
 }
 
 static int param_get_aalockpolicy(char *buffer, const struct kernel_param *kp)
 {
-	if (!policy_view_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_view_capable(NULL))
+		return -EPERM;
 	return param_get_bool(buffer, kp);
 }
 
 static int param_set_aabool(const char *val, const struct kernel_param *kp)
 {
-	if (!policy_admin_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_admin_capable(NULL))
+		return -EPERM;
 	return param_set_bool(val, kp);
 }
 
 static int param_get_aabool(char *buffer, const struct kernel_param *kp)
 {
-	if (!policy_view_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_view_capable(NULL))
+		return -EPERM;
 	return param_get_bool(buffer, kp);
 }
 
 static int param_set_aauint(const char *val, const struct kernel_param *kp)
 {
-	if (!policy_admin_capable(NULL))
-		return -EPERM;
+	int error;
+
 	if (!apparmor_enabled)
 		return -EINVAL;
-	return param_set_uint(val, kp);
+	if (apparmor_initialized && !policy_admin_capable(NULL))
+		return -EPERM;
+
+	error = param_set_uint(val, kp);
+	pr_info("AppArmor: buffer size set to %d bytes\n", aa_g_path_max);
+
+	return error;
 }
 
 static int param_get_aauint(char *buffer, const struct kernel_param *kp)
 {
-	if (!policy_view_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_view_capable(NULL))
+		return -EPERM;
 	return param_get_uint(buffer, kp);
 }
 
 static int param_get_audit(char *buffer, struct kernel_param *kp)
 {
-	if (!policy_view_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_view_capable(NULL))
+		return -EPERM;
 	return sprintf(buffer, "%s", audit_mode_names[aa_g_audit]);
 }
 
 static int param_set_audit(const char *val, struct kernel_param *kp)
 {
 	int i;
-	if (!policy_admin_capable(NULL))
-		return -EPERM;
+
 	if (!apparmor_enabled)
 		return -EINVAL;
 	if (!val)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_admin_capable(NULL))
+		return -EPERM;
 
 	for (i = 0; i < AUDIT_MAX_INDEX; i++) {
 		if (strcmp(val, audit_mode_names[i]) == 0) {
@@ -1490,10 +1499,10 @@  static int param_set_audit(const char *val, struct kernel_param *kp)
 
 static int param_get_mode(char *buffer, struct kernel_param *kp)
 {
-	if (!policy_view_capable(NULL))
-		return -EPERM;
 	if (!apparmor_enabled)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_view_capable(NULL))
+		return -EPERM;
 
 	return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]);
 }
@@ -1501,12 +1510,13 @@  static int param_get_mode(char *buffer, struct kernel_param *kp)
 static int param_set_mode(const char *val, struct kernel_param *kp)
 {
 	int i;
-	if (!policy_admin_capable(NULL))
-		return -EPERM;
+
 	if (!apparmor_enabled)
 		return -EINVAL;
 	if (!val)
 		return -EINVAL;
+	if (apparmor_initialized && !policy_admin_capable(NULL))
+		return -EPERM;
 
 	for (i = 0; i < APPARMOR_MODE_NAMES_MAX_INDEX; i++) {
 		if (strcmp(val, aa_profile_mode_names[i]) == 0) {