diff mbox series

device_cgroup: Roll back to original exceptions after copy failure

Message ID 20221025113101.41132-1-wangweiyang2@huawei.com
State New
Headers
Series device_cgroup: Roll back to original exceptions after copy failure |

Commit Message

Wang Weiyang Oct. 25, 2022, 11:31 a.m. UTC 
  When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
exceptions will be cleaned and A's behavior is changed to
DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
whitelist. If copy failure occurs, just return leaving A to grant
permissions to all devices. And A may grant more permissions than
parent.

Backup A's whitelist and recover original exceptions after copy
failure.

Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
---
 security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

Comments

Paul Moore Oct. 28, 2022, 11:19 a.m. UTC | #1 
On Tue, Oct 25, 2022 at 7:02 AM Wang Weiyang <wangweiyang2@huawei.com> wrote:
>
> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
> exceptions will be cleaned and A's behavior is changed to
> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
> whitelist. If copy failure occurs, just return leaving A to grant
> permissions to all devices. And A may grant more permissions than
> parent.
>
> Backup A's whitelist and recover original exceptions after copy
> failure.
>
> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
> ---
>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)

On quick glance this looks reasonable to me, but I'm working with
limited time connected to a network so I can't say I've given this a
full and proper review; if a third party could spend some time to give
this an additional review before I merge it I would greatly appreciate
it.

> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index a9f8c63a96d1..bef2b9285fb3 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -82,6 +82,17 @@ static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
>         return -ENOMEM;
>  }
>
> +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig)
> +{
> +       struct dev_exception_item *ex, *tmp;
> +
> +       lockdep_assert_held(&devcgroup_mutex);
> +
> +       list_for_each_entry_safe(ex, tmp, orig, list) {
> +               list_move_tail(&ex->list, dest);
> +       }
> +}
> +
>  /*
>   * called under devcgroup_mutex
>   */
> @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>         int count, rc = 0;
>         struct dev_exception_item ex;
>         struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent);
> +       struct dev_cgroup tmp_devcgrp;
>
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;
>
>         memset(&ex, 0, sizeof(ex));
> +       memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp));
>         b = buffer;
>
>         switch (*b) {
> @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>
>                         if (!may_allow_all(parent))
>                                 return -EPERM;
> -                       dev_exception_clean(devcgroup);
> -                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> -                       if (!parent)
> +                       if (!parent) {
> +                               devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> +                               dev_exception_clean(devcgroup);
>                                 break;
> +                       }
>
> +                       INIT_LIST_HEAD(&tmp_devcgrp.exceptions);
> +                       rc = dev_exceptions_copy(&tmp_devcgrp.exceptions,
> +                                                &devcgroup->exceptions);
> +                       if (rc)
> +                               return rc;
> +                       dev_exception_clean(devcgroup);
>                         rc = dev_exceptions_copy(&devcgroup->exceptions,
>                                                  &parent->exceptions);
> -                       if (rc)
> +                       if (rc) {
> +                               dev_exceptions_move(&devcgroup->exceptions,
> +                                                   &tmp_devcgrp.exceptions);
>                                 return rc;
> +                       }
> +                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> +                       dev_exception_clean(&tmp_devcgrp);
>                         break;
>                 case DEVCG_DENY:
>                         if (css_has_online_children(&devcgroup->css))
> --
> 2.17.1
>
Aristeu Rozanski Nov. 7, 2022, 6:56 p.m. UTC | #2 
On Tue, Oct 25, 2022 at 07:31:01PM +0800, Wang Weiyang wrote:
> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
> exceptions will be cleaned and A's behavior is changed to
> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
> whitelist. If copy failure occurs, just return leaving A to grant
> permissions to all devices. And A may grant more permissions than
> parent.
> 
> Backup A's whitelist and recover original exceptions after copy
> failure.
> 
> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
> ---
>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index a9f8c63a96d1..bef2b9285fb3 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -82,6 +82,17 @@ static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
>  	return -ENOMEM;
>  }
>  
> +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig)
> +{
> +	struct dev_exception_item *ex, *tmp;
> +
> +	lockdep_assert_held(&devcgroup_mutex);
> +
> +	list_for_each_entry_safe(ex, tmp, orig, list) {
> +		list_move_tail(&ex->list, dest);
> +	}
> +}
> +
>  /*
>   * called under devcgroup_mutex
>   */
> @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>  	int count, rc = 0;
>  	struct dev_exception_item ex;
>  	struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent);
> +	struct dev_cgroup tmp_devcgrp;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
>  	memset(&ex, 0, sizeof(ex));
> +	memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp));
>  	b = buffer;
>  
>  	switch (*b) {
> @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>  
>  			if (!may_allow_all(parent))
>  				return -EPERM;
> -			dev_exception_clean(devcgroup);
> -			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> -			if (!parent)
> +			if (!parent) {
> +				devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> +				dev_exception_clean(devcgroup);
>  				break;
> +			}
>  
> +			INIT_LIST_HEAD(&tmp_devcgrp.exceptions);
> +			rc = dev_exceptions_copy(&tmp_devcgrp.exceptions,
> +						 &devcgroup->exceptions);
> +			if (rc)
> +				return rc;
> +			dev_exception_clean(devcgroup);
>  			rc = dev_exceptions_copy(&devcgroup->exceptions,
>  						 &parent->exceptions);
> -			if (rc)
> +			if (rc) {
> +				dev_exceptions_move(&devcgroup->exceptions,
> +						    &tmp_devcgrp.exceptions);
>  				return rc;
> +			}
> +			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> +			dev_exception_clean(&tmp_devcgrp);
>  			break;
>  		case DEVCG_DENY:
>  			if (css_has_online_children(&devcgroup->css))

Reviewed-by: Aristeu Rozanski <aris@redhat.com>
Wang Weiyang Nov. 15, 2022, 3:54 a.m. UTC | #3 
Hi, Paul
Can this patch be applied or something to improve?

Thanks

on 2022/10/28 19:19, Paul Moore wrote:
> On Tue, Oct 25, 2022 at 7:02 AM Wang Weiyang <wangweiyang2@huawei.com> wrote:
>>
>> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
>> exceptions will be cleaned and A's behavior is changed to
>> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
>> whitelist. If copy failure occurs, just return leaving A to grant
>> permissions to all devices. And A may grant more permissions than
>> parent.
>>
>> Backup A's whitelist and recover original exceptions after copy
>> failure.
>>
>> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
>> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
>> ---
>>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
>>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> On quick glance this looks reasonable to me, but I'm working with
> limited time connected to a network so I can't say I've given this a
> full and proper review; if a third party could spend some time to give
> this an additional review before I merge it I would greatly appreciate
> it.
> 
>> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
>> index a9f8c63a96d1..bef2b9285fb3 100644
>> --- a/security/device_cgroup.c
>> +++ b/security/device_cgroup.c
>> @@ -82,6 +82,17 @@ static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
>>         return -ENOMEM;
>>  }
>>
>> +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig)
>> +{
>> +       struct dev_exception_item *ex, *tmp;
>> +
>> +       lockdep_assert_held(&devcgroup_mutex);
>> +
>> +       list_for_each_entry_safe(ex, tmp, orig, list) {
>> +               list_move_tail(&ex->list, dest);
>> +       }
>> +}
>> +
>>  /*
>>   * called under devcgroup_mutex
>>   */
>> @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>>         int count, rc = 0;
>>         struct dev_exception_item ex;
>>         struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent);
>> +       struct dev_cgroup tmp_devcgrp;
>>
>>         if (!capable(CAP_SYS_ADMIN))
>>                 return -EPERM;
>>
>>         memset(&ex, 0, sizeof(ex));
>> +       memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp));
>>         b = buffer;
>>
>>         switch (*b) {
>> @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
>>
>>                         if (!may_allow_all(parent))
>>                                 return -EPERM;
>> -                       dev_exception_clean(devcgroup);
>> -                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>> -                       if (!parent)
>> +                       if (!parent) {
>> +                               devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>> +                               dev_exception_clean(devcgroup);
>>                                 break;
>> +                       }
>>
>> +                       INIT_LIST_HEAD(&tmp_devcgrp.exceptions);
>> +                       rc = dev_exceptions_copy(&tmp_devcgrp.exceptions,
>> +                                                &devcgroup->exceptions);
>> +                       if (rc)
>> +                               return rc;
>> +                       dev_exception_clean(devcgroup);
>>                         rc = dev_exceptions_copy(&devcgroup->exceptions,
>>                                                  &parent->exceptions);
>> -                       if (rc)
>> +                       if (rc) {
>> +                               dev_exceptions_move(&devcgroup->exceptions,
>> +                                                   &tmp_devcgrp.exceptions);
>>                                 return rc;
>> +                       }
>> +                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>> +                       dev_exception_clean(&tmp_devcgrp);
>>                         break;
>>                 case DEVCG_DENY:
>>                         if (css_has_online_children(&devcgroup->css))
>> --
>> 2.17.1
>>
> 
>
Paul Moore Nov. 15, 2022, 8:55 p.m. UTC | #4 
On Mon, Nov 14, 2022 at 10:54 PM wangweiyang <wangweiyang2@huawei.com> wrote:
>
> Hi, Paul
> Can this patch be applied or something to improve?

Possibly.  I need to find the time to properly review the patch; it's
in my queue, I just haven't gotten to it yet.  I'll reply to the patch
posting with either a note that the patch has been merged, or a set of
questions/feedback that needs to be resolved.  If you haven't heard
from me on the patch, the best thing you can do is exercise patience.

> on 2022/10/28 19:19, Paul Moore wrote:
> > On Tue, Oct 25, 2022 at 7:02 AM Wang Weiyang <wangweiyang2@huawei.com> wrote:
> >>
> >> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
> >> exceptions will be cleaned and A's behavior is changed to
> >> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
> >> whitelist. If copy failure occurs, just return leaving A to grant
> >> permissions to all devices. And A may grant more permissions than
> >> parent.
> >>
> >> Backup A's whitelist and recover original exceptions after copy
> >> failure.
> >>
> >> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
> >> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
> >> ---
> >>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
> >>  1 file changed, 29 insertions(+), 4 deletions(-)
> >
> > On quick glance this looks reasonable to me, but I'm working with
> > limited time connected to a network so I can't say I've given this a
> > full and proper review; if a third party could spend some time to give
> > this an additional review before I merge it I would greatly appreciate
> > it.
> >
> >> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> >> index a9f8c63a96d1..bef2b9285fb3 100644
> >> --- a/security/device_cgroup.c
> >> +++ b/security/device_cgroup.c
> >> @@ -82,6 +82,17 @@ static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
> >>         return -ENOMEM;
> >>  }
> >>
> >> +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig)
> >> +{
> >> +       struct dev_exception_item *ex, *tmp;
> >> +
> >> +       lockdep_assert_held(&devcgroup_mutex);
> >> +
> >> +       list_for_each_entry_safe(ex, tmp, orig, list) {
> >> +               list_move_tail(&ex->list, dest);
> >> +       }
> >> +}
> >> +
> >>  /*
> >>   * called under devcgroup_mutex
> >>   */
> >> @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
> >>         int count, rc = 0;
> >>         struct dev_exception_item ex;
> >>         struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent);
> >> +       struct dev_cgroup tmp_devcgrp;
> >>
> >>         if (!capable(CAP_SYS_ADMIN))
> >>                 return -EPERM;
> >>
> >>         memset(&ex, 0, sizeof(ex));
> >> +       memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp));
> >>         b = buffer;
> >>
> >>         switch (*b) {
> >> @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
> >>
> >>                         if (!may_allow_all(parent))
> >>                                 return -EPERM;
> >> -                       dev_exception_clean(devcgroup);
> >> -                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> >> -                       if (!parent)
> >> +                       if (!parent) {
> >> +                               devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> >> +                               dev_exception_clean(devcgroup);
> >>                                 break;
> >> +                       }
> >>
> >> +                       INIT_LIST_HEAD(&tmp_devcgrp.exceptions);
> >> +                       rc = dev_exceptions_copy(&tmp_devcgrp.exceptions,
> >> +                                                &devcgroup->exceptions);
> >> +                       if (rc)
> >> +                               return rc;
> >> +                       dev_exception_clean(devcgroup);
> >>                         rc = dev_exceptions_copy(&devcgroup->exceptions,
> >>                                                  &parent->exceptions);
> >> -                       if (rc)
> >> +                       if (rc) {
> >> +                               dev_exceptions_move(&devcgroup->exceptions,
> >> +                                                   &tmp_devcgrp.exceptions);
> >>                                 return rc;
> >> +                       }
> >> +                       devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> >> +                       dev_exception_clean(&tmp_devcgrp);
> >>                         break;
> >>                 case DEVCG_DENY:
> >>                         if (css_has_online_children(&devcgroup->css))
> >> --
> >> 2.17.1
> >>
> >
> >
Paul Moore Nov. 16, 2022, 11:33 p.m. UTC | #5 
On Tue, Oct 25, 2022 at 7:02 AM Wang Weiyang <wangweiyang2@huawei.com> wrote:
>
> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
> exceptions will be cleaned and A's behavior is changed to
> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
> whitelist. If copy failure occurs, just return leaving A to grant
> permissions to all devices. And A may grant more permissions than
> parent.
>
> Backup A's whitelist and recover original exceptions after copy
> failure.
>
> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
> ---
>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)

Merged into lsm/next, but with a stable@vger tag.  Normally I would
merge something like this into lsm/stable-X.Y and send it up to Linus
after a few days, but I'd really like this to spend some time in
linux-next before going up to Linus.
Wang Weiyang Nov. 17, 2022, 7:27 a.m. UTC | #6 
On 2022/11/17 7:33, Paul Moore wrote:
> On Tue, Oct 25, 2022 at 7:02 AM Wang Weiyang <wangweiyang2@huawei.com> wrote:
>>
>> When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's
>> exceptions will be cleaned and A's behavior is changed to
>> DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's
>> whitelist. If copy failure occurs, just return leaving A to grant
>> permissions to all devices. And A may grant more permissions than
>> parent.
>>
>> Backup A's whitelist and recover original exceptions after copy
>> failure.
>>
>> Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior")
>> Signed-off-by: Wang Weiyang <wangweiyang2@huawei.com>
>> ---
>>  security/device_cgroup.c | 33 +++++++++++++++++++++++++++++----
>>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> Merged into lsm/next, but with a stable@vger tag.  Normally I would
> merge something like this into lsm/stable-X.Y and send it up to Linus
> after a few days, but I'd really like this to spend some time in
> linux-next before going up to Linus.

Thanks Paul. This sounds fine.
diff mbox series

Patch

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index a9f8c63a96d1..bef2b9285fb3 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -82,6 +82,17 @@  static int dev_exceptions_copy(struct list_head *dest, struct list_head *orig)
 	return -ENOMEM;
 }
 
+static void dev_exceptions_move(struct list_head *dest, struct list_head *orig)
+{
+	struct dev_exception_item *ex, *tmp;
+
+	lockdep_assert_held(&devcgroup_mutex);
+
+	list_for_each_entry_safe(ex, tmp, orig, list) {
+		list_move_tail(&ex->list, dest);
+	}
+}
+
 /*
  * called under devcgroup_mutex
  */
@@ -604,11 +615,13 @@  static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 	int count, rc = 0;
 	struct dev_exception_item ex;
 	struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent);
+	struct dev_cgroup tmp_devcgrp;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
 	memset(&ex, 0, sizeof(ex));
+	memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp));
 	b = buffer;
 
 	switch (*b) {
@@ -620,15 +633,27 @@  static int devcgroup_update_access(struct dev_cgroup *devcgroup,
 
 			if (!may_allow_all(parent))
 				return -EPERM;
-			dev_exception_clean(devcgroup);
-			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
-			if (!parent)
+			if (!parent) {
+				devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+				dev_exception_clean(devcgroup);
 				break;
+			}
 
+			INIT_LIST_HEAD(&tmp_devcgrp.exceptions);
+			rc = dev_exceptions_copy(&tmp_devcgrp.exceptions,
+						 &devcgroup->exceptions);
+			if (rc)
+				return rc;
+			dev_exception_clean(devcgroup);
 			rc = dev_exceptions_copy(&devcgroup->exceptions,
 						 &parent->exceptions);
-			if (rc)
+			if (rc) {
+				dev_exceptions_move(&devcgroup->exceptions,
+						    &tmp_devcgrp.exceptions);
 				return rc;
+			}
+			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+			dev_exception_clean(&tmp_devcgrp);
 			break;
 		case DEVCG_DENY:
 			if (css_has_online_children(&devcgroup->css))