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 DC85BC369DC for ; Tue, 29 Apr 2025 17:12:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 454EB6B0008; Tue, 29 Apr 2025 13:12:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 42A2D6B000A; Tue, 29 Apr 2025 13:12:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2F3FE6B000C; Tue, 29 Apr 2025 13:12:50 -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 10B946B0008 for ; Tue, 29 Apr 2025 13:12:50 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id A6A41BFE4B for ; Tue, 29 Apr 2025 17:12:50 +0000 (UTC) X-FDA: 83387726100.29.92CB39B Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by imf26.hostedemail.com (Postfix) with ESMTP id B3ACE140003 for ; Tue, 29 Apr 2025 17:12:48 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YA6dZ9z6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1745946768; a=rsa-sha256; cv=none; b=Pcky+gfvywAIFeDv9rOK6GAM/h9LVrpKRM7lR6zd5pbB5f7KINgRzHwlPUs+NhFYL1pnjK OVePM6tgXfwyFn3XMEiPZ7oN9hUjfbvhgUASawz/iJeuXxtYk12shEoRbHVqXyNx4BCHj7 E+FqJ+JM/U2Hn8wM3EhLZRybqueMB8U= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YA6dZ9z6; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf26.hostedemail.com: domain of mjguzik@gmail.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=mjguzik@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1745946768; 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-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=SsfvUz+i23ivaruHjbfi9ou4xDoXYHDfMl7yFqsBNNY=; b=cyZpXDd6T4r2+x6l5XGz8yUd6zyZ7kDmUxeMr/18ySKceM8U3c+cb/8jM1QQ1Vnx8S1xm+ KEj2VjpRpZQLmATIsH0svzMqiHI1oM41SYy4SY1D+DETjQxJ034uXwmYU/ljsOaBOwjS9g /bMvFbQQdgIlaxmxCScoCqaNt7j9xNg= Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-5f6222c6c4cso9837884a12.1 for ; Tue, 29 Apr 2025 10:12:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1745946767; x=1746551567; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=SsfvUz+i23ivaruHjbfi9ou4xDoXYHDfMl7yFqsBNNY=; b=YA6dZ9z6bN9RqxfyVC7J5mqXp3QlztAJRpgGCHmoovIVrpi9rF4Q7yepduzsUDzKRM QRYxWyCAlIg39DBpQh6V0L6yS/fWu4gqeuONgehgGeFk+Sid/q+Ulr4T8fIdnYT0Qmxs 7w8+k0/Yvgk5GD9aovlpjXn9e/e0MLfNtyGZAeZ5sAZ2aiYk+jVj4XPPhfy4VWYgw0Ru G9mQYJ1QnZmE/MarJPNT5he5JKOa1174GriQN9SWFedpUGbqfGjWCXp/UkvWd7117JqL A2Lt4p0A+Fnzdqz76bgu07hvvYnxhQxalvsM7xIFcbdmhRzlsvmuGHn9bjrMFq3vXIST wXOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1745946767; x=1746551567; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SsfvUz+i23ivaruHjbfi9ou4xDoXYHDfMl7yFqsBNNY=; b=FeGc4GbcczshanDfCHa0us7zkPOI6XdgJ4M3E9/zE84zfzeYH73HH7q7DLIUQUx8kR h0fbqHfR6DTA20E47DFlqlCbxes0GuX19zuwn5+QcVCoDVR665wdmBnmxrQDp+O5RY4F 8KRfBvadvsSR3743l+rS1B1tnkJKn2383zP7M6yFGnnXgeUIune8FX7ChXKz9LdNzq4T dEdrZvOyzX/aO/wbb+0HcO0UlBhpyKpKOOVuYKLhWqoCEdQA3cslAOja1IUZxuDbFLeo XWqx9FNQRkXF7mg5gOOMIQBkCbVE/vVFLlhvESEwVN+okIRJFJKOCZoxPNGRAajZorlI dXAQ== X-Forwarded-Encrypted: i=1; AJvYcCX5cuio5Whp3AMQfteZYI7iUWEDTv4DuY5nme/jgkxvou+wtidFL8DM+vSZJFC/CFgwP3LvdvG9JA==@kvack.org X-Gm-Message-State: AOJu0YxXHU5XL6ynnQnkXOVJ6rr08KQEtnW6+ZmAcMEscRP362+RJDos ewwdi70nA96HrKJhr08sUzj9QvhbHRK2yyShJGTC2BLhrK9QQ3fE8HP+kDahgjmSAzyGt9bBqOH fDpZfs6p1txVi6OEhMcNHeW2uG1k= X-Gm-Gg: ASbGncvlSerDWCfxOO92BfyVfRh/+arXvzEw5lmfAx4o3p9JQvMdq/hKPR6MAHnBjNh /3+eM6CLDOpxPLvQgxosYN2e/30zPWszLDmWTjhedSHHxYyU8xG/jXyEzlk2KMUyV6W86V/GcFj ZHWdiIb/XX1ilKWnc1SLC9 X-Google-Smtp-Source: AGHT+IEFhOpRJtHhdameANKEr4F4uoVUaEZAhtAMKro2joPY0ijis9Vj5f2jUhCYho2E3q3oqxjBEekqlySWOza8xhU= X-Received: by 2002:a05:6402:370c:b0:5f4:d60f:93f0 with SMTP id 4fb4d7f45d1cf-5f83889e7bamr3636129a12.31.1745946766822; Tue, 29 Apr 2025 10:12:46 -0700 (PDT) MIME-Version: 1.0 References: <67dc67f0.050a0220.25ae54.001f.GAE@google.com> <20250324160003.GA8878@redhat.com> <20250429154944.GA18907@redhat.com> In-Reply-To: <20250429154944.GA18907@redhat.com> From: Mateusz Guzik Date: Tue, 29 Apr 2025 19:12:34 +0200 X-Gm-Features: ATxdqUEx3Vi0APNX--GO3y5NvRxsG1VNhvPaOpzMVNW8AhLlmPepqTW-UkJkA5E Message-ID: Subject: Re: [PATCH] exec: fix the racy usage of fs_struct->in_exec To: Oleg Nesterov Cc: brauner@kernel.org, kees@kernel.org, viro@zeniv.linux.org.uk, jack@suse.cz, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, syzkaller-bugs@googlegroups.com, Michal Hocko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: B3ACE140003 X-Stat-Signature: o4wi5nt7hrd8dunn4773mx348zo3z9gw X-HE-Tag: 1745946768-255055 X-HE-Meta: U2FsdGVkX18OQDHcJ1xwP5Oa3pdlU+Rri+mHzcM7PpnLFIKNBSmH503CcG5byX9JgMCYcPJC/JEUqUzBeHkRXgkkb7/0jlv7AxVYtCS9NaueV0dI5mheHRnB3yrug8IuJGHmr38v2Z/tNbSff4cFxPurIBI3Y0C+kkmIxZfII7iagBm2DdkGko98soRaC+S+ENV3KQ3ZFsBQQjK9DguICEADwcnrwdBVtQw73cLu76S9HvSq09HAK0uYLss1AaEKGmWCi4nLXVxpc62qzMDvFVEUhscbYqLWzeamWyrnpUjTeVCLNhW3UZWzg2iOJ36gUS6f9LZ/aTLMzuFmC8Adff08X7/5GkS6T+xhV7i1R8bp3+IBLuIcafGJnoxcyKs4VkT/L4WOgRwCgoAe/4mwZM4c0U5NTBlsMbR9xu7bjhJ8PpdB1d5wER1PzHvAgNEEohCHR7X2af+PQILFlKz5ekHaaNBBFKAsytunbWmsiv7Go4PGBuSp7n6aYGKVEUT9A326WcmdRVgL0/kUGU9wU/1vs2r4D76HUWceh2GaprRB7cjKDl/xDrxhuVyk3rGHBYwJH5vSTHDQhIeCeUxrXCr6xpi7G393FBvLI5Sqcy+jEASl7wkQRShBDMI0z90tW1gD2+WRUTtZaPG5lr/4n3WavbcqOGtQOIyUciLoqHXXMg8bkDmXVU/mEvqMySeIEpQ09VAffGNSwss8Wl5BUQwxzmSa2d9xYqN4Dp0IiJeP/1a/+5wkjTBqGiKqRw1NwrPquPXzsXMJWxzRoe0kU5zwi2e3ioFEKLAuJHtjluFYjXPh56nyRf0UNvgIOldr51CK5nQxYtRfM8k4qV8OuLoUj3dWddsRtWYQQ7nGRaVC4AANi45tkQYJzI1v5QegIsRM35oamBHUMpgdK5ThP9sUQBZIkPZv+Qp6jQqfEfhdjoyBEQAMf/5bytqOObJ6f9KVBYaOVSHQSSQpDcE b/MB70cP B6H2SDB+iCmTN4wKkKqys6vGIDEBR5odZhvf334vu/txXja5myDO5DokhUU2M7CduVNfy/qao6ZYUH3+dcDsY6ptw7vO3r8sgmQjU2Pa2oiZquTRdFDOTAbhnyKnlVy2QnrRnT55g1Wn79rge3gq4e/pzy7iCz12NaQTofyTNc7nXeUtsvgXjBlaDqeTHu+q4IHWbRDrJE8bsrFTtmctwGtulhqfQzgrShPoPT1saGzKXOIA3us7kuyCI1MnRY7hoDkjQ9f/Szpc9W1/X1IsaTXIFZFj/aDrCPnEX4H5LXMo4qW3kMRRYJHPYBA33DBdmDeGt3bEtoSzozHqWO4MXDLpbAbO5MlHCJqLkTjisrrRShuoFDpMsm4WH9osDVTes6sfnbFdgz2gy8L5huOpz6WeB7Ft3eEtMJyzEqAg0cmZGB1Dqbhv2CZQ/YlJxjCRZm+dNQltpQpaTRvc= 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: List-Subscribe: List-Unsubscribe: On Tue, Apr 29, 2025 at 5:50=E2=80=AFPM Oleg Nesterov wro= te: > > Damn, I am stupid. > > On 03/24, Oleg Nesterov wrote: > > > > check_unsafe_exec() sets fs->in_exec under cred_guard_mutex, then execv= e() > > paths clear fs->in_exec lockless. This is fine if exec succeeds, but if= it > > fails we have the following race: > > > > T1 sets fs->in_exec =3D 1, fails, drops cred_guard_mutex > > > > T2 sets fs->in_exec =3D 1 > > > > T1 clears fs->in_exec > > When I look at this code again, I think this race was not possible and th= us > this patch (applied as af7bb0d2ca45) was not needed. > > Yes, begin_new_exec() can drop cred_guard_mutex on failure, but only afte= r > de_thread() succeeds, when we can't race with another sub-thread. > > I hope this patch didn't make the things worse so we don't need to revert= it. > Plus I think it makes this (confusing) logic a bit more clear. Just, unle= ss > I am confused again, it wasn't really needed. > > -------------------------------------------------------------------------= ---- > But. I didn't read the original report from syzbot, > https://lore.kernel.org/all/67dc67f0.050a0220.25ae54.001f.GAE@google.com/= #t > because I wasn't CC'ed. and then - sorry Kees!!! - I didn't bother to rea= d > your first reply carefully. > > So yes, with or without this patch the "if (fs->in_exec)" check in copy_f= s() > can obviously hit the 1 -> 0 transition. > > This is harmless, but should be probably fixed just to avoid another repo= rt > from KCSAN. > > I do not want to add another spin_lock(fs->lock). We can change copy_fs()= to > use data_race(), but I'd prefer the patch below. Yes, it needs the additi= onal > comment(s) to explain READ_ONCE(). > > What do you think? Did I miss somthing again??? Quite possibly... > > Mateusz, I hope you will cleanup this horror sooner or later ;) > > Oleg. > --- > > diff --git a/fs/exec.c b/fs/exec.c > index 5d1c0d2dc403..42a7f9b43911 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1495,7 +1495,7 @@ static void free_bprm(struct linux_binprm *bprm) > free_arg_pages(bprm); > if (bprm->cred) { > /* in case exec fails before de_thread() succeeds */ > - current->fs->in_exec =3D 0; > + WRITE_ONCE(current->fs->in_exec, 0); > mutex_unlock(¤t->signal->cred_guard_mutex); > abort_creds(bprm->cred); > } > diff --git a/kernel/fork.c b/kernel/fork.c > index 4c2df3816728..381af8c8ece8 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1802,7 +1802,7 @@ static int copy_fs(unsigned long clone_flags, struc= t task_struct *tsk) > /* tsk->fs is already what we want */ > spin_lock(&fs->lock); > /* "users" and "in_exec" locked for check_unsafe_exec() *= / > - if (fs->in_exec) { > + if (READ_ONCE(fs->in_exec)) { > spin_unlock(&fs->lock); > return -EAGAIN; > } > good grief man ;) I have this on my TODO list, (un)fortunately $life got in the way and by now I swapped out almost all of the context. I mostly remember the code is hard to follow. ;) that said, maybe i'll give it a stab soon(tm). I have a testcase somewhere to validate that this does provide the expect behavior vs suid, so it's not going to be me flying blindly. --=20 Mateusz Guzik