Subject: [merged] coredump-set_dumpable-fix-the-theoretical-race-with-itself.patch removed from -mm tree To: oleg@xxxxxxxxxx,alex.page.kelly@xxxxxxxxx,ebiederm@xxxxxxxxxxxx,josh@xxxxxxxxxxxxxxxx,keescook@xxxxxxxxxxxx,pmatouse@xxxxxxxxxx,segoon@xxxxxxxxxxxx,mm-commits@xxxxxxxxxxxxxxx From: akpm@xxxxxxxxxxxxxxxxxxxx Date: Fri, 24 Jan 2014 11:03:24 -0800 The patch titled Subject: coredump: set_dumpable: fix the theoretical race with itself has been removed from the -mm tree. Its filename was coredump-set_dumpable-fix-the-theoretical-race-with-itself.patch This patch was dropped because it was merged into mainline or a subsystem tree ------------------------------------------------------ From: Oleg Nesterov <oleg@xxxxxxxxxx> Subject: coredump: set_dumpable: fix the theoretical race with itself 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@xxxxxxxxxx> Acked-by: Kees Cook <keescook@xxxxxxxxxxxx> Cc: Alex Kelly <alex.page.kelly@xxxxxxxxx> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx> Cc: Petr Matousek <pmatouse@xxxxxxxxxx> Cc: Vasily Kulikov <segoon@xxxxxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> --- fs/exec.c | 49 +++++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff -puN fs/exec.c~coredump-set_dumpable-fix-the-theoretical-race-with-itself fs/exec.c --- a/fs/exec.c~coredump-set_dumpable-fix-the-theoretical-race-with-itself +++ a/fs/exec.c @@ -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) { - 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; - } + unsigned long old, new; + + do { + old = ACCESS_ONCE(mm->flags); + new = old & ~MMF_DUMPABLE_MASK; + + switch (value) { + case SUID_DUMP_ROOT: + 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) _ Patches currently in -mm which might be from oleg@xxxxxxxxxx are origin.patch mm-fix-the-theoretical-compound_lock-vs-prep_new_page-race.patch -- To unsubscribe from this list: send the line "unsubscribe mm-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html