David Laight <David.Laight@xxxxxxxxxx> writes:
> From: Andy Lutomirski
>> Sent: 14 October 2022 04:18
> ...
>> But seriously, this makes no sense at all. It should not be possible to exec a program and then,
>> without ptrace, change its cwd out from under it. Do we really need to preserve this behavior?
>
> it maybe ok if the exec'ed program also 'bought-in' to the
> fact that its cwd and open files might get changed.
> But imagine someone doing it to a login shell!
I am slowly catching up on my email and I saw this conversation.
When I initially saw this thread I was confused and thought this
might run into an issue with fs/locks.c. I was close but wrong.
fs/locks.c uses current->files as a sort of process identifier
and so is very sensitive to when it is unshared. Making
unsharing current->files unconditionally a bug. Not relevant to
this conversation.
There are several clone options that were only relevant for the old
LinuxThreads implementation including CLONE_FS and CLONE_SIGHAND.
The LinuxThreads implementation has not been needed since
the introduction of CLONE_THREAD in linux-2.6.0 in 17 Dec 2003.
Almost 20 years ago.
I suggest we introduce CONFIG_CLONE_FS and CONFIG_SIGHAND to allow
disabling support of these clone options. No known user space will
care. The are both getting in the way of kernel maintenance so there
is a reason to start pushing them out.
Further simply not worrying about UNSHARE_FS during exec fixes the
race so it essentially a bug fix by code removal.
I believe something like the patch below should get the job done.
diff --git a/fs/exec.c b/fs/exec.c
index a0b1f0337a62..7ff13c77ad04 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1186,7 +1186,8 @@ static int unshare_sighand(struct task_struct *me)
{
struct sighand_struct *oldsighand = me->sighand;
- if (refcount_read(&oldsighand->count) != 1) {
+ if (IS_ENABLED(CONFIG_CLONE_SIGHAND) &&
+ refcount_read(&oldsighand->count) != 1) {
struct sighand_struct *newsighand;
/*
* This ->sighand is shared with the CLONE_SIGHAND
@@ -1568,6 +1569,9 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
if (task_no_new_privs(current))
bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
+ if (!IS_ENABLED(CONFIG_CLONE_FS))
+ return;
+
t = p;
n_fs = 1;
spin_lock(&p->fs->lock);
diff --git a/init/Kconfig b/init/Kconfig
index 94125d3b6893..8660a6bcc1cf 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1764,6 +1764,23 @@ config KALLSYMS_BASE_RELATIVE
time constants, and no relocation pass is required at runtime to fix
up the entries based on the runtime load address of the kernel.
+config CLONE_FS
+ bool
+ default y
+ help
+ Support CLONE_FS being passed to clone. The only known user
+ is the old LinuxThreads package so it should be safe to disable
+ this option.
+
+config CLONE_SIGHAND
+ bool
+ default y
+ help
+ Support CLONE_SIGHAND being passed to clone. The only known user
+ is the old LinuxThreads package so it should be safe to disable
+ this option.
+
+
# end of the "standard kernel features (expert users)" menu
# syscall, maps, verifier
diff --git a/kernel/fork.c b/kernel/fork.c
index 08969f5aa38d..da9017b51da4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2023,6 +2023,16 @@ static __latent_entropy struct task_struct *copy_process(
if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))
return ERR_PTR(-EINVAL);
+ /* Don't allow CLONE_FS if not enabled */
+ if (!IS_ENABLED(CONFIG_CLONE_FS) &&
+ ((clone_flags & (CLONE_THREAD | CLONE_FS)) == CLONE_FS))
+ return ERR_PTR(-EINVAL);
+
+ /* Don't allow CLONE_SIGHAND if not enabled */
+ if (!IS_ENABLED(CONFIG_CLONE_SIGHAND) &&
+ ((clone_flags & (CLONE_THREAD | CLONE_SIGHAND)) == CLONE_SIGHAND))
+ return ERR_PTR(-EINVAL);
+
/*
* Siblings of global init remain as zombies on exit since they are
* not reaped by their parent (swapper). To solve this and to avoid
@@ -3101,6 +3111,9 @@ static int unshare_fs(unsigned long unshare_flags, struct fs_struct **new_fsp)
if (fs->users == 1)
return 0;
+ if (!IS_ENABLED(CONFIG_CLONE_FS))
+ return -EINVAL;
+
*new_fsp = copy_fs_struct(fs);
if (!*new_fsp)
return -ENOMEM;
Eric
