From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp-42ac.mail.infomaniak.ch (smtp-42ac.mail.infomaniak.ch [84.16.66.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 15763193066 for ; Wed, 10 Jul 2024 13:53:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=84.16.66.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720619608; cv=none; b=F61UH0ZtvQdOOPotV0MdOj2dIDWPgWP4Gsft1smCLl2WM5ERHDkZEgva7MZWOq26n6OIoFQT36KaMEpG6BF0ZJlcdyP8tU14K2GvrrMhNmSiBixwVo6jdw3TJk9XMI79N1unVkkjSYYPzcVfZwSR5gulA0VF3pCLaDAFTwM9Bvc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720619608; c=relaxed/simple; bh=5Q6phVlqBjRBZPPTioIgE049kVnw2FiAlwyhm369ZKs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ATiZ06HSdA6inKtR7EfWoN9tYUo6VVlgzEXOoixBCkI6mOkifFYpFB87YAYPsftxkszHABbmsES9dg3xXXzRFVPbTg12bmO0OIntj2M59KyfuXzxeAXdFrBDUZDo+sGBB8V4ukfaRQj/3ZZ5jXzLTkUdgMMjhFfuvajQOLV7lX4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net; spf=pass smtp.mailfrom=digikod.net; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b=V3xs9yDp; arc=none smtp.client-ip=84.16.66.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=digikod.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b="V3xs9yDp" Received: from smtp-4-0000.mail.infomaniak.ch (smtp-4-0000.mail.infomaniak.ch [10.7.10.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4WJzpV0VXGzbm7; Wed, 10 Jul 2024 15:53:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=digikod.net; s=20191114; t=1720619601; bh=ifwDgxqKwDaPE0SIqSz4qw8wsXTHuLeU/PsJiOUMJI4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=V3xs9yDpY344VxfBbhOk7n7ClxvATSRgjyBrefapCSdQxKBHlKp20p5Lrl/j3Z13r WbZFJfaNKdkJ7KSL8DRU4B+u1d/PRHfH1eo0sCOG5wg7TQl8/iWmQHW9S6pzudpa0k eOiBmJJZMHEQ5mHKk0yGuuXqJL8ojGjx35ZlRq7A= Received: from unknown by smtp-4-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4WJzpS0lVGzcvq; Wed, 10 Jul 2024 15:53:20 +0200 (CEST) Date: Wed, 10 Jul 2024 15:53:16 +0200 From: =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= To: Paul Moore Cc: Jann Horn , Christian Brauner , "Paul E. McKenney" , Casey Schaufler , Kees Cook , syzbot , jmorris@namei.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, serge@hallyn.com, syzkaller-bugs@googlegroups.com, linux-fsdevel@vger.kernel.org, Mimi Zohar , Roberto Sassu Subject: Re: [syzbot] [lsm?] general protection fault in hook_inode_free_security Message-ID: <20240710.Aephuhain8lu@digikod.net> References: <00000000000076ba3b0617f65cc8@google.com> <20240515.Yoo5chaiNai9@digikod.net> <20240516.doyox6Iengou@digikod.net> <20240627.Voox5yoogeum@digikod.net> <20240710.Hai0Uj3Phaij@digikod.net> Precedence: bulk X-Mailing-List: linux-security-module@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240710.Hai0Uj3Phaij@digikod.net> X-Infomaniak-Routing: alpha On Wed, Jul 10, 2024 at 02:23:23PM +0200, Mickaël Salaün wrote: > On Thu, Jun 27, 2024 at 02:28:03PM -0400, Paul Moore wrote: > > On Thu, Jun 27, 2024 at 9:34 AM Mickaël Salaün wrote: > > > > > > I didn't find specific issues with Landlock's code except the extra > > > check in hook_inode_free_security(). It looks like inode->i_security is > > > a dangling pointer, leading to UAF. > > > > > > Reading security_inode_free() comments, two things looks weird to me: > > > > > > > /** > > > > * security_inode_free() - Free an inode's LSM blob > > > > * @inode: the inode > > > > * > > > > * Deallocate the inode security structure and set @inode->i_security to NULL. > > > > > > I don't see where i_security is set to NULL. > > > > The function header comments are known to be a bit suspect, a side > > effect of being detached from the functions for many years, this may > > be one of those cases. I tried to fix up the really awful ones when I > > moved the comments back, back I didn't have time to go through each > > one in detail. Patches to correct the function header comments are > > welcome and encouraged! :) > > > > > > */ > > > > void security_inode_free(struct inode *inode) > > > > { > > > > > > Shouldn't we add this check here? > > > if (!inode->i_security) > > > return; > > > > Unless I'm remembering something wrong, I believe we *should* always > > have a valid i_security pointer each time we are called, if not > > something has gone wrong, e.g. the security_inode_free() hook is no > > longer being called from the right place. If we add a NULL check, we > > should probably have a WARN_ON(), pr_err(), or something similar to > > put some spew on the console/logs. > > > > All that said, it would be good to hear some confirmation from the VFS > > folks that the security_inode_free() hook is located in a spot such > > that once it exits it's current RCU critical section it is safe to > > release the associated LSM state. > > > > It's also worth mentioning that while we always allocate i_security in > > security_inode_alloc() right now, I can see a world where we allocate > > the i_security field based on need using the lsm_blob_size info (maybe > > that works today? not sure how kmem_cache handled 0 length blobs?). > > The result is that there might be a legitimate case where i_security > > is NULL, yet we still want to call into the LSM using the > > inode_free_security() implementation hook. > > Looking at existing LSM implementations, even if some helpers (e.g. > selinux_inode) return NULL if inode->i_security is NULL, this may not be > handled by the callers. For instance, SELinux always dereferences the > blob pointer in the security_inode_permission() hook. EVM seems to be > the only one properly handling this case. > > Shouldn't we remove all inode->i_security checks and assume it is always > set? This is currently the case anyway, but it would be clearer this > way and avoid false sense of security (with useless checks). A patch was sent to do this kind of check: https://lore.kernel.org/r/20140109101932.0508dec7@gandalf.local.home but the applied commit 3dc91d4338d6 ("SELinux: Fix possible NULL pointer dereference in selinux_inode_permission()") didn't include the i_security check. Since this commit, the security_inode_free()'s header comment is no longer correct because i_security is no longer set to NULL.