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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 0A5D6C4345F for ; Thu, 2 May 2024 23:03:27 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 754A010E4E0; Thu, 2 May 2024 23:03:27 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="nsscsV0s"; dkim-atps=neutral Received: from mail-pl1-f182.google.com (mail-pl1-f182.google.com [209.85.214.182]) by gabe.freedesktop.org (Postfix) with ESMTPS id E6DE110F740 for ; Thu, 2 May 2024 23:03:25 +0000 (UTC) Received: by mail-pl1-f182.google.com with SMTP id d9443c01a7336-1e5715a9ebdso72612375ad.2 for ; Thu, 02 May 2024 16:03:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1714691005; x=1715295805; darn=lists.freedesktop.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=I4RW3z/Zjpo6qwpklLmujf0x6xZ5b4UXLeUz+5BU7aE=; b=nsscsV0s4CSpDHku+Q0THxGAL1LY3wxCEFpuwHtt0AUcLVzL6jR90nYmHASzcSkHUu K+LD9pJLtOGqMr5zdWz2R9GTK+k18rAyIhL7RAM93H/P6uG90jxQdrBFPwCjhRmmh8x5 2ohOTR/O0J2VPWXfZQPrrgoGMEY1jD0pPM+HY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714691005; x=1715295805; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=I4RW3z/Zjpo6qwpklLmujf0x6xZ5b4UXLeUz+5BU7aE=; b=dAIlDzovcKQfGUGryZZVrjus9m224iLq2Fpy+GfrdeGvDP92KwR2Oz1m8PfkoO0aua FkxDdc4JgkQSy+qVLqMTPJ7Gt8G88cmK4XdQvfIkwa9NTUf2z0qGrw0xgl2/irsf6cVP o0lr9osva2/RDjFyGtkTFzP6CTcY8fTby3Ay6ynX2X/KkB5sN2i6JZdYxmDegh2i8qN8 Dgz6vCx6hxoZ5QixVJN5bFEKHcpdnCbmivFFlfFf64jq/mz/04S/JH4ltB5w+50jk4Gw nMMduvaHPA+/xEy8JhRca5UYPKCr/ckFkRVLNyt9tNbwEXGcAwO55DF6VRkTvGeDfDbo znIQ== X-Forwarded-Encrypted: i=1; AJvYcCXU/khcyyrxONfIMr38gn/orfKQIqPZi2cnuyeqeafkFaQreAWFIis0+X4iY9F9Hm7yFrHXHcuKJBdbqIzztotrUOtUA0HEftrqFRPkMHc4 X-Gm-Message-State: AOJu0YzmtsDh6aodo71KI+dXOOPssraF8bnHiri8edghklp8MC9zE7jE ccGbo3HufWm94X82Mo7BPADCKFpXH2uHJ+New0SsNOC7+J8icaHMbEaiSSVArg== X-Google-Smtp-Source: AGHT+IFN1ghcNnx1vVRkm1yx/61qx4Z1G+3b7jaqkQpPQe3ATJr7d5YMS5BN39H3i6ElpFem8xw9lw== X-Received: by 2002:a17:90a:c70a:b0:2b2:7e94:c5fa with SMTP id o10-20020a17090ac70a00b002b27e94c5famr1292136pjt.5.1714691005284; Thu, 02 May 2024 16:03:25 -0700 (PDT) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id gn23-20020a17090ac79700b002b2b608c10esm1807658pjb.56.2024.05.02.16.03.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 May 2024 16:03:24 -0700 (PDT) Date: Thu, 2 May 2024 16:03:24 -0700 From: Kees Cook To: Jann Horn Cc: Christian Brauner , Alexander Viro , Jan Kara , linux-fsdevel@vger.kernel.org, Zack Rusin , Broadcom internal kernel review list , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Tvrtko Ursulin , Andi Shyti , Lucas De Marchi , Matt Atwood , Matthew Auld , Nirmoy Das , Jonathan Cavitt , Will Deacon , Peter Zijlstra , Boqun Feng , Mark Rutland , Kent Overstreet , Masahiro Yamada , Nathan Chancellor , Nicolas Schier , Andrew Morton , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kbuild@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH 1/5] fs: Do not allow get_file() to resurrect 0 f_count Message-ID: <202405021600.F5C68084D@keescook> References: <20240502222252.work.690-kees@kernel.org> <20240502223341.1835070-1-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, May 03, 2024 at 12:53:56AM +0200, Jann Horn wrote: > On Fri, May 3, 2024 at 12:34 AM Kees Cook wrote: > > If f_count reaches 0, calling get_file() should be a failure. Adjust to > > use atomic_long_inc_not_zero() and return NULL on failure. In the future > > get_file() can be annotated with __must_check, though that is not > > currently possible. > [...] > > static inline struct file *get_file(struct file *f) > > { > > - atomic_long_inc(&f->f_count); > > + if (unlikely(!atomic_long_inc_not_zero(&f->f_count))) > > + return NULL; > > return f; > > } > > Oh, I really don't like this... > > In most code, if you call get_file() on a file and see refcount zero, > that basically means you're in a UAF write situation, or that you > could be in such a situation if you had raced differently. It's > basically just like refcount_inc() in that regard. Shouldn't the system attempt to not make things worse if it encounters an inc-from-0 condition? Yes, we've already lost the race for a UaF condition, but maybe don't continue on. > And get_file() has semantics just like refcount_inc(): The caller > guarantees that it is already holding a reference to the file; and if Yes, but if that guarantee is violated, we should do something about it. > the caller is wrong about that, their subsequent attempt to clean up > the reference that they think they were already holding will likely > lead to UAF too. If get_file() sees a zero refcount, there is no safe > way to continue. And all existing callers of get_file() expect the > return value to be the same as the non-NULL pointer they passed in, so > they'll either ignore the result of this check and barrel on, or oops > with a NULL deref. > > For callers that want to actually try incrementing file refcounts that > could be zero, which is only possible under specific circumstances, we > have helpers like get_file_rcu() and get_file_active(). So what's going on in here: https://lore.kernel.org/linux-hardening/20240502223341.1835070-2-keescook@chromium.org/ > Can't you throw a CHECK_DATA_CORRUPTION() or something like that in > there instead? I'm open to suggestions, but given what's happening with struct dma_buf above, it seems like this is a state worth checking for? -- Kees Cook