coredump: set_dumpable: fix the theoretical race with itself (abacd2fe) · Commits · 戴 / test

set_dumpable() updates MMF_DUMPABLE_MASK in a non-trivial way to ensure that get_dumpable() can't observe the intermediate state, but this all can't help if multiple threads call set_dumpable() at the same time. And in theory commit_creds()->set_dumpable(SUID_DUMP_ROOT) racing with sys_prctl()->set_dumpable(SUID_DUMP_DISABLE) can result in SUID_DUMP_USER. Change this code to update both bits atomically via cmpxchg(). Note: this assumes that it is safe to mix bitops and cmpxchg. IOW, if, say, an architecture implements cmpxchg() using the locking (like arch/parisc/lib/bitops.c does), then it should use the same locks for set_bit/etc. Signed-off-by:

Oleg Nesterov <oleg@redhat.com> Acked-by:

Kees Cook <keescook@chromium.org> Cc: Alex Kelly <alex.page.kelly@gmail.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Josh Triplett <josh@joshtriplett.org> Cc: Petr Matousek <pmatouse@redhat.com> Cc: Vasily Kulikov <segoon@openwall.com> Signed-off-by:

Andrew Morton <akpm@linux-foundation.org> Signed-off-by:

Linus Torvalds <torvalds@linux-foundation.org>

fs/exec.c

+15 −34

Original line number	Diff line number	Diff line
		@@ -1614,43 +1614,24 @@ EXPORT_SYMBOL(set_binfmt);

		/*
		* set_dumpable converts traditional three-value dumpable to two flags and
		* stores them into mm->flags. It modifies lower two bits of mm->flags, but
		* these bits are not changed atomically. So get_dumpable can observe the
		* intermediate state. To avoid doing unexpected behavior, get get_dumpable
		* return either old dumpable or new one by paying attention to the order of
		* modifying the bits.
		*
		* dumpable \| mm->flags (binary)
		* old new \| initial interim final
		* ---------+-----------------------
		* 0 1 \| 00 01 01
		* 0 2 \| 00 10(*) 11
		* 1 0 \| 01 00 00
		* 1 2 \| 01 11 11
		* 2 0 \| 11 10(*) 00
		* 2 1 \| 11 11 01
		*
		* (*) get_dumpable regards interim value of 10 as 11.
		* stores them into mm->flags.
		*/
		void set_dumpable(struct mm_struct *mm, int value)
		{
		unsigned long old, new;

		do {
		old = ACCESS_ONCE(mm->flags);
		new = old & ~MMF_DUMPABLE_MASK;

		switch (value) {
		case SUID_DUMP_DISABLE:
		clear_bit(MMF_DUMPABLE, &mm->flags);
		smp_wmb();
		clear_bit(MMF_DUMP_SECURELY, &mm->flags);
		break;
		case SUID_DUMP_USER:
		set_bit(MMF_DUMPABLE, &mm->flags);
		smp_wmb();
		clear_bit(MMF_DUMP_SECURELY, &mm->flags);
		break;
		case SUID_DUMP_ROOT:
		set_bit(MMF_DUMP_SECURELY, &mm->flags);
		smp_wmb();
		set_bit(MMF_DUMPABLE, &mm->flags);
		break;
		new \|= (1 << MMF_DUMP_SECURELY);
		case SUID_DUMP_USER:
		new \|= (1<< MMF_DUMPABLE);
		}

		} while (cmpxchg(&mm->flags, old, new) != old);
		}

		int __get_dumpable(unsigned long mm_flags)

Admin message