-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
[EMAIL PROTECTED] wrote:
| Quoting Andrew G. Morgan ([EMAIL PROTECTED]):
| Here is my latest per-process secure-bits patch.
|
|> Hey Andrew,
|
|> looks really good. Two comments inline.
Thanks for the review!
- - unsigned keep_capabilities:1;
+ unsigned securebits;
| should this maybe be an unsigned short?
I'm not sure what this would buy us. Given the placement in the data
structure its going to end up aligned by the surrounding data types and
will occupy at least a sizeof(unsigned) amount of space even if we
declare it as a short. Could you clarify why you think a short would be
better?
+ case PR_GET_KEEPCAPS:
+ if (issecure(SECURE_KEEP_CAPS))
+ error = 1;
+ break;
+ case PR_SET_KEEPCAPS:
+ if ((arg2 > 1) && !issecure(SECURE_KEEP_CAPS_LOCKED))
| I don't understand what you're doing here. If I had to guess, I'd say
| you're only enforcing a check on a valid arg2 (which is 0 or 1) only if
| SECURE_KEEP_CAPS_LOCKED is not set since otherwise the value you can't
| be changed? But you don't enforce that so it can in fact be changed,
| and even if you did this seems to give the user poor feedback in more than
| one case, so it seems nicer to do
I think the long and the short of it was that I clearly got distracted
when writing that code! It didn't help that all of my testing was with
the PR_*_SECUREBITS prctl()... What I had meant to write was this:
case PR_SET_KEEPCAPS:
/* Note, we rely on arg2 being unsigned here: */
if ((arg2 > 1) || issecure(SECURE_KEEP_CAPS_LOCKED))
error = -EINVAL;
else
current->securebits
= (current->securebits
& ~issecure_mask(SECURE_KEEP_CAPS))
| (arg2 << SECURE_KEEP_CAPS);
break;
| if (arg2 > 1)
| return -EINVAL;
| if (issecure(SECURE_KEEP_CAPS_LOCKED))
| return -EPERM;
+ error = -EINVAL;
+ else
+ current->securebits
+ = (current->securebits
+ & ~issecure_mask(SECURE_KEEP_CAPS))
+ | (arg2 * issecure_mask(SECURE_KEEP_CAPS));
| Seems overly baroque, and since you're not checking arg2>1 if
| SECURE_KEEP_CAPS_LOCKED was set, this could actually set a wrong bit
| with the multiplication. After the above checks a simple
| if (arg2)
| current->securebits |= issecure_mask(SECURE_KEEP_CAPS);
| else
| current->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
| would be much easier to read...
I agree. Both that -EPERM is a better error for the locked case, and the
subsequent if / else. So, I'll use that.
Thanks
Andrew
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
iD8DBQFHoBN0+bHCR3gb8jsRAqRkAJ99ABdBV6iP61MR/OEqmyj+aKOhrACcDuT2
Qm89xx7i9/wOEPLoOASFZHs=
=Zao3
-----END PGP SIGNATURE-----
-
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
