[patch] fix NPTL thread iterator construct in cap_set_pg()

[patch] fix NPTL thread iterator construct in cap_set_pg()

[Ken Chen]

The usage of while_each_pid_task() construct in cap_set_pg() looks incorrect.
The macro is meant to form 'do ... while' loop instead of a simple while loop.
I think currently it will skip thread leader of a NPTL process.

Fix by convert to 'do ... while' style.

Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/kernel/capability.c b/kernel/capability.c
index 0101e84..26d8eda 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -167,7 +167,7 @@ static inline int cap_set_pg
 	pgrp = find_vpid(pgrp_nr);
 	do_each_pid_task(pgrp, PIDTYPE_PGID, g) {
 		target = g;
-		while_each_thread(g, target) {
+		do {
 			if (!security_capset_check(target, effective,
 						   inheritable, permitted)) {
 				security_capset_set(target, effective,
@@ -175,7 +175,7 @@ static inline int cap_set_pg
 				ret = 0;
 			}
 			found = 1;
-		}
+		} while_each_thread(g, target);
 	} while_each_pid_task(pgrp, PIDTYPE_PGID, g);

 	read_unlock(&tasklist_lock);

Re: [patch] fix NPTL thread iterator construct in cap_set_pg()

[Andrew G. Morgan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ken,

In 2.2.0, pre-LSM and NPTL, this code looked like this:

     read_lock(&tasklist_lock);
     for_each_task(target) {
             if (target->pgrp != pgrp)
                     continue;
             target->cap_effective   = *effective;
             target->cap_inheritable = *inheritable;
             target->cap_permitted   = *permitted;
     }
     read_unlock(&tasklist_lock);

that is, if a process is in the process group, it got its capabilities
changed.

So far as I can tell, I've not touched this code since it looked like
the above, so I can't really comment on the motivation for the thread
iterator changes. I'm also going to have to read up on NPTL to say
anything credible on this front.

In general, its very hard to get a system state in which this code is
run, so it wouldn't surprise me if these thread iterator changes have
never been tested.

Finally, I've very much in favor of deleting any code that modifies the
capabilities of another process/thread; see the #if[n]def
CONFIG_SECURITY_FILE_CAPABILITIES sections of this file, so any fix for
this (which, if needed, should be adopted) may also be short-lived...

Cheers

Andrew

Ken Chen wrote:
> The usage of while_each_pid_task() construct in cap_set_pg() looks incorrect.
> The macro is meant to form 'do ... while' loop instead of a simple while loop.
> I think currently it will skip thread leader of a NPTL process.
> 
> Fix by convert to 'do ... while' style.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)

iD8DBQFIqLRO+bHCR3gb8jsRAnFGAJoDm9VxcpmOYqEuaWmaVrnNWkb9owCgzOPH
6tG3n4GMsW8oNX4MiNiJ3FQ=
=obJY
-----END PGP SIGNATURE-----

Re: [patch] fix NPTL thread iterator construct in cap_set_pg()

[Andrew Morton]

On Fri, 15 Aug 2008 09:24:15 -0700
"Ken Chen" <kenchen@google.com> wrote:

> The usage of while_each_pid_task() construct in cap_set_pg() looks incorrect.
> The macro is meant to form 'do ... while' loop instead of a simple while loop.
> I think currently it will skip thread leader of a NPTL process.
> 
> Fix by convert to 'do ... while' style.
> 
> Signed-off-by: Ken Chen <kenchen@google.com>
> 
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 0101e84..26d8eda 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -167,7 +167,7 @@ static inline int cap_set_pg
>  	pgrp = find_vpid(pgrp_nr);
>  	do_each_pid_task(pgrp, PIDTYPE_PGID, g) {
>  		target = g;
> -		while_each_thread(g, target) {
> +		do {
>  			if (!security_capset_check(target, effective,
>  						   inheritable, permitted)) {
>  				security_capset_set(target, effective,
> @@ -175,7 +175,7 @@ static inline int cap_set_pg
>  				ret = 0;
>  			}
>  			found = 1;
> -		}
> +		} while_each_thread(g, target);
>  	} while_each_pid_task(pgrp, PIDTYPE_PGID, g);
> 
>  	read_unlock(&tasklist_lock);

cap_set_pg() gets deleted by the credentials patches in linux-next and
afaict nothing replaced it.

Does this patch actually fix anything?  If not, ignoring it would be a
nice labour-saving device...

Re: [patch] fix NPTL thread iterator construct in cap_set_pg()

[Ken Chen]

On Thu, Aug 21, 2008 at 12:18 PM, Andrew Morton wrote:
> cap_set_pg() gets deleted by the credentials patches in linux-next and
> afaict nothing replaced it.
>
> Does this patch actually fix anything?  If not, ignoring it would be a
> nice labour-saving device...

No, the patch was a result of code inspection.  There wasn't anything
that affected anyone.  If the function cap_set_pg() is destined for
deletion, yeah I agree there is no point of fixing it.

- Ken