Quoting Andrew G. Morgan ([EMAIL PROTECTED]): > -----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?
Is that true on all architectures? I figured there must be some architecture where it isn't, but if not then never mind. > > + 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, -serge - 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
