[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAG48ez2LBmS91fQVLYRYEaBHssj22NyUjB0HVtkDHUXDvDZ6EA@mail.gmail.com>
Date: Thu, 15 Aug 2024 21:59:54 +0200
From: Jann Horn <jannh@...gle.com>
To: David Howells <dhowells@...hat.com>
Cc: Jeffrey Altman <jaltman@...istor.com>, openafs-devel@...nafs.org,
Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>, John Johansen <john.johansen@...onical.com>,
Jarkko Sakkinen <jarkko@...nel.org>, Mickaël Salaün <mic@...ikod.net>,
Günther Noack <gnoack@...gle.com>,
Stephen Smalley <stephen.smalley.work@...il.com>, Ondrej Mosnacek <omosnace@...hat.com>,
Casey Schaufler <casey@...aufler-ca.com>, linux-afs@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
apparmor@...ts.ubuntu.com, keyrings@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: Can KEYCTL_SESSION_TO_PARENT be dropped entirely? -- was Re:
[PATCH v2 1/2] KEYS: use synchronous task work for changing parent credentials
On Thu, Aug 15, 2024 at 9:46 PM David Howells <dhowells@...hat.com> wrote:
> Jann Horn <jannh@...gle.com> wrote:
>
> > Rewrite keyctl_session_to_parent() to run task work on the parent
> > synchronously, so that any errors that happen in the task work can be
> > plumbed back into the syscall return value in the child.
>
> The main thing I worry about is if there's a way to deadlock the child and the
> parent against each other. vfork() for example.
Yes - I think it would work fine for scenarios like using
KEYCTL_SESSION_TO_PARENT from a helper binary against the shell that
launched the helper (which I think is the intended usecase?), but
there could theoretically be constellations where it would cause an
(interruptible) hang if the parent is stuck in
uninterruptible/killable sleep.
I think vfork() is rather special in that it does a killable wait for
the child to exit or execute; and based on my understanding of the
intended usecase of KEYCTL_SESSION_TO_PARENT, I think normally
KEYCTL_SESSION_TO_PARENT would only be used by a child that has gone
through execve?
> > + if (task_work_cancel(parent, &ctx.work)) {
> > + /*
> > + * We got interrupted and the task work was canceled before it
> > + * could execute.
> > + * Use -ERESTARTNOINTR instead of -ERESTARTSYS for
> > + * compatibility - the manpage does not list -EINTR as a
> > + * possible error for keyctl().
> > + */
>
> I think returning EINTR is fine, provided that if we return EINTR, the change
> didn't happen. KEYCTL_SESSION_TO_PARENT is only used by the aklog, dlog and
> klog* OpenAFS programs AFAIK, and only if "-setpag" is set as a command line
> option. It also won't be effective if you strace the program.
Ah, I didn't even know about those.
The users I knew of are the command-line tools "keyctl new_session"
and "e4crypt new_session" (see
https://codesearch.debian.net/search?q=KEYCTL_SESSION_TO_PARENT&literal=1,
which indexes code that's part of Debian).
> Maybe the AFS people can say whether it's even worth keeping the functionality
> rather than just dropping KEYCTL_SESSION_TO_PARENT?
I think this would break the tools "keyctl new_session" and "e4crypt
new_session" - though I don't know if anyone actually uses those
invocations.
Powered by blists - more mailing lists