On Wed, Jul 31, 2024 at 11:33 PM Jann Horn [off-list ref] wrote:

On Wed, Jul 31, 2024 at 11:27 PM Paul Moore [off-list ref] wrote:

quoted

On Wed, Jul 31, 2024 at 4:54 PM Jann Horn [off-list ref] wrote:

quoted

FYI: Those checks, including the hook that formerly existed there, are
(somewhat necessarily) racy wrt concurrent security context changes of
the parent because they come before asynchronous work is posted to the
parent to do the keyring update.

I was wondering about something similar while looking at
keyctl_session_to_parent(), aren't all of the parent's cred checks
here racy?

Yeah...

quoted

quoted

In theory we could make them synchronous if we have the child wait for
the parent to enter task work... actually, with that we could probably
get rid of the whole cred_transfer hack and have the parent do
prepare_creds() and commit_creds() normally, and propagate any errors
back to the child, as long as we don't create any deadlocks with
this...

Assuming that no problems are caused by waiting on the parent, this
might be the best approach.  Should we also move, or duplicate, the
cred checks into the parent's context to avoid any races?

Yeah, I think that'd probably be a reasonable way to do it. Post task
work to the parent, wait for the task work to finish (with an
interruptible sleep that cancels the work item on EINTR), and then do
the checks and stuff in the parent. I guess whether we should also do
racy checks in the child before that depends on whether we're worried
about a child without the necessary permissions being able to cause
spurious syscall restarts in the parent...

I hacked up an RFC patch for this approach:
https://lore.kernel.org/r/20240802-remove-cred-transfer-v1-1-b3fef1ef2ade@google.com (local)