From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2FDFEC6FA82 for ; Sat, 10 Sep 2022 21:13:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2DA798D0001; Sat, 10 Sep 2022 17:13:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 28A516B0072; Sat, 10 Sep 2022 17:13:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 12C148D0001; Sat, 10 Sep 2022 17:13:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 01C166B0071 for ; Sat, 10 Sep 2022 17:13:43 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C265FC020B for ; Sat, 10 Sep 2022 21:13:43 +0000 (UTC) X-FDA: 79897427526.23.6BE8FA5 Received: from smtp-relay-internal-0.canonical.com (smtp-relay-internal-0.canonical.com [185.125.188.122]) by imf09.hostedemail.com (Postfix) with ESMTP id 238511400A2 for ; Sat, 10 Sep 2022 21:13:43 +0000 (UTC) Received: from mail-oo1-f70.google.com (mail-oo1-f70.google.com [209.85.161.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 95D9441300 for ; Sat, 10 Sep 2022 21:13:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1662844420; bh=YIXqxtSbGe6vWCX7g0eeSDhgSl7mx2fz9QQkdP6SCjU=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=Q0TwgsaOSFCRJguD1ghJ2rz87+8dJtC/6PUu80y/+w7mehDloMDkS206iTGW6QNk2 jc7zsxTJzIt83cpummD/rKIHUhB4H9Dpt8sE5nuUms0wO2SJ9+4SnueAjb7B/YTCf1 z8Jfji/SauUDGO4VM7h7EA5MXpojMfK9+BlfAj6dEFQR5HKRCfZC+dNS2gqHfVWYfP l9eRuv/vUdfPJjOwLNRjhmkvnT7+ldU67J7RfDQ2/gAzBMpzNPH8qr65VYmaOX/TA2 49R/nkroN3l1teezI48AjIcB6B9EL1gV2NiIZ2L+puUZJ4oeYH4OwV/sjEPEOAg3xI im0sxb9fdZM1A== Received: by mail-oo1-f70.google.com with SMTP id c27-20020a4a9c5b000000b0044df4a6e6f4so2218073ook.15 for ; Sat, 10 Sep 2022 14:13:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date; bh=YIXqxtSbGe6vWCX7g0eeSDhgSl7mx2fz9QQkdP6SCjU=; b=RMutFNg0M880g16vZNMV7E/0NbJUneEXtxOBfLhTBIelZiplmGSo+32MbPi1XnTIYp q0E00gSk9ZtsR6MQtd1nWqQq1098/GdOu8jixC38mVDpCtzdSMCzW687HZ6Zk26Phwju CZhSn0qOVO5BI2WcsfjkI1xzWlK4MH6jRwoAXUYZC4awRHpWly9dFInYKKYnV8ZRvMIJ qPCFgEL59EoXOMdcG19VB7+JeV2biOiUmj58SS7A9wTcHjuvx+XJwyq1IrsxZMAE4ixj pAz01pFuqSe2fdqbJKjCDtwvDhwWDwd4VGSO8rE5I3nTZSlIM1j6k33JV/QsbpRWGkRO xANA== X-Gm-Message-State: ACgBeo0rnhiBsjIlZs3N0mMlGpzMt4osMBhhLIBeUQCiYgWDCyG90Dy+ gUjbTnKN2HBkfCQ07Y+57hapy0PlSrGjMEFVR9ZJZvnNo6y88Pm4VBp5kQV3kT0jk/Bv0LAjr+R kZb/w+KdxsBRCPTRzomkgQDed45Mw X-Received: by 2002:a05:6830:1f2e:b0:655:ca83:ddf2 with SMTP id e14-20020a0568301f2e00b00655ca83ddf2mr1151070oth.235.1662844414861; Sat, 10 Sep 2022 14:13:34 -0700 (PDT) X-Google-Smtp-Source: AA6agR5pGsEEItr/aSLMNOIU+AZmK8M+PFn+4mg2dcqIyP1lfX4BseZAUG23BVX+s27yE6qxA8684g== X-Received: by 2002:a05:6830:1f2e:b0:655:ca83:ddf2 with SMTP id e14-20020a0568301f2e00b00655ca83ddf2mr1151054oth.235.1662844414569; Sat, 10 Sep 2022 14:13:34 -0700 (PDT) Received: from localhost.localdomain ([2001:67c:1562:8007::aac:4084]) by smtp.gmail.com with ESMTPSA id z14-20020a056870e30e00b0012769122387sm2892567oad.54.2022.09.10.14.13.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Sep 2022 14:13:34 -0700 (PDT) From: Jorge Merlino To: Alexander Viro , Eric Biederman , Kees Cook , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Daniel Bristot de Oliveira , Valentin Schneider Cc: Jorge Merlino , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: [PATCH] Fix race condition when exec'ing setuid files Date: Sat, 10 Sep 2022 18:12:14 -0300 Message-Id: <20220910211215.140270-1-jorge.merlino@canonical.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662844423; a=rsa-sha256; cv=none; b=1VXJxqafrP7UMvAM1xeZ/yTqOEKaS5BusWVjHTwJaDa7SWMRtKnmTcUkIyW2kZuOHY15f6 p18X7cgdT7broIkly95cMX3heBUb+jh7yFsR78KI8Wa0YYWapZqfFXe/wEgY7dBXrcQjdL fR52v0O2Jjo+bfDK7iLhVghnxEsvaTw= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=canonical.com header.s=20210705 header.b=Q0TwgsaO; spf=pass (imf09.hostedemail.com: domain of jorge.merlino@canonical.com designates 185.125.188.122 as permitted sender) smtp.mailfrom=jorge.merlino@canonical.com; dmarc=pass (policy=none) header.from=canonical.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662844423; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-transfer-encoding:content-transfer-encoding: in-reply-to:references:dkim-signature; bh=YIXqxtSbGe6vWCX7g0eeSDhgSl7mx2fz9QQkdP6SCjU=; b=wxQcsX4bvJj9Aa/Q02bB9eNd1YVn98SSNxrCXo5jzOp0aWsctDHOj/72sIVh4bufZ5zpvE sYMZZmKyhkXrTrnSK6t3xV6RrafxSZ/xxSsMfLkRtFbpuIPiAHuE+okx/7Eck/PQBRjTTe +SNzElPuefnBIvX7ZuzDtOgY3h4T0Ms= X-Stat-Signature: 4qjeajqm9f4cysaxndz1qtyq9atpydg6 X-Rspamd-Queue-Id: 238511400A2 X-Rspam-User: Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=canonical.com header.s=20210705 header.b=Q0TwgsaO; spf=pass (imf09.hostedemail.com: domain of jorge.merlino@canonical.com designates 185.125.188.122 as permitted sender) smtp.mailfrom=jorge.merlino@canonical.com; dmarc=pass (policy=none) header.from=canonical.com X-Rspamd-Server: rspam07 X-HE-Tag: 1662844422-120837 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: This patch fixes a race condition in check_unsafe_exec when a heavily threaded program tries to exec a setuid file. check_unsafe_exec counts the number of threads sharing the same fs_struct and compares it to the total number of users of the fs_struct by looking at its users counter. If there are more users than process threads using it the setuid exec fails with LSM_UNSAFE_SHARE. The problem is that, during the kernel_clone code execution, the fs_struct users counter is incremented before the new thread is added to the thread_group list. So there is a race when the counter has been incremented but the thread is not visible to the while_each_tread loop in check_unsafe_exec. This patch sort of fixes this by setting a process flag to the parent process during the time this race is possible. Thus, if a process is forking, it counts an extra user fo the fs_struct as the counter might be incremented before the thread is visible. But this is not great as this could generate the opposite problem as there may be an external process sharing the fs_struct that is masked by some thread that is being counted twice. I submit this patch just as an idea but mainly I want to introduce this issue and see if someone comes up with a better solution. This is a simple code to reproduce this issue: $ cat Makefile ALL=a b all: $(ALL) a: LDFLAGS=-pthread b: b.c $(CC) b.c -o b sudo chown root:root b sudo chmod u+s b test: for I in $$(seq 1000); do echo $I; ./a ; done clean: rm -vf $(ALL) $ cat a.c void *nothing(void *p) { return NULL; } void *target(void *p) { for (;;) { pthread_t t; if (pthread_create(&t, NULL, nothing, NULL) == 0) pthread_join(t, NULL); } return NULL; } int main(void) { struct timespec tv; int i; for (i = 0; i < 10; i++) { pthread_t t; pthread_create(&t, NULL, target, NULL); } tv.tv_sec = 0; tv.tv_nsec = 100000; nanosleep(&tv, NULL); if (execl("./b", "./b", NULL) < 0) perror("execl"); return 0; } $ cat b.c int main(void) { const uid_t euid = geteuid(); if (euid != 0) { printf("Failed, got euid %d (expecting 0)\n", euid); return 1; } return 0; } $ make make cc -pthread a.c -o a cc b.c -o b sudo chown root:root b sudo chmod u+s b $ make test Without this fix, one will see 'Failed, got euid 1000 (expecting 0)' messages --- fs/exec.c | 2 ++ include/linux/sched.h | 1 + kernel/fork.c | 3 +++ 3 files changed, 6 insertions(+) diff --git a/fs/exec.c b/fs/exec.c index 9a5ca7b82bfc..a6f949a899d5 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1581,6 +1581,8 @@ static void check_unsafe_exec(struct linux_binprm *bprm) while_each_thread(p, t) { if (t->fs == p->fs) n_fs++; + if (t->flags & PF_IN_FORK) + n_fs++; } rcu_read_unlock(); diff --git a/include/linux/sched.h b/include/linux/sched.h index e7b2f8a5c711..f307165a434a 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1722,6 +1722,7 @@ extern struct pid *cad_pid; * I am cleaning dirty pages from some other bdi. */ #define PF_KTHREAD 0x00200000 /* I am a kernel thread */ #define PF_RANDOMIZE 0x00400000 /* Randomize virtual address space */ +#define PF_IN_FORK 0x02000000 /* Process is forking, prevents race condition on fs_struct users value */ #define PF_NO_SETAFFINITY 0x04000000 /* Userland is not allowed to meddle with cpus_mask */ #define PF_MCE_EARLY 0x08000000 /* Early kill for mce process policy */ #define PF_MEMALLOC_PIN 0x10000000 /* Allocation context constrained to zones which allow long term pinning. */ diff --git a/kernel/fork.c b/kernel/fork.c index 8a9e92068b15..54e1e1fbe0bd 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2245,6 +2245,7 @@ static __latent_entropy struct task_struct *copy_process( retval = copy_files(clone_flags, p); if (retval) goto bad_fork_cleanup_semundo; + current->flags |= PF_IN_FORK; retval = copy_fs(clone_flags, p); if (retval) goto bad_fork_cleanup_files; @@ -2474,6 +2475,7 @@ static __latent_entropy struct task_struct *copy_process( attach_pid(p, PIDTYPE_PID); nr_threads++; } + current->flags &= ~PF_IN_FORK; total_forks++; hlist_del_init(&delayed.node); spin_unlock(¤t->sighand->siglock); @@ -2556,6 +2558,7 @@ static __latent_entropy struct task_struct *copy_process( spin_lock_irq(¤t->sighand->siglock); hlist_del_init(&delayed.node); spin_unlock_irq(¤t->sighand->siglock); + current->flags &= ~PF_IN_FORK; return ERR_PTR(retval); } -- 2.34.1