Received-SPF: pass (google.com: domain of linux-kernel+bounces-122865-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 28 Mar 2024 13:07:33 +0100
From: Christian Brauner <brauner@kernel.org>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>, 
	Al Viro <viro@zeniv.linux.org.uk>, Steve French <smfrench@gmail.com>, 
	LKML <linux-kernel@vger.kernel.org>, linux-fsdevel <linux-fsdevel@vger.kernel.org>, 
	CIFS <linux-cifs@vger.kernel.org>, Paulo Alcantara <pc@manguebit.com>, 
	Christian Brauner <christian@brauner.io>, Mimi Zohar <zohar@linux.ibm.com>, 
	Paul Moore <paul@paul-moore.com>, 
	"linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>, 
	"linux-security-module@vger.kernel.org" <linux-security-module@vger.kernel.org>
Subject: Re: kernel crash in mknod
Message-ID: <20240328-verfrachten-geebnet-19181fb9ad65@brauner>
References: <CAH2r5msAVzxCUHHG8VKrMPUKQHmBpE6K9_vjhgDa1uAvwx4ppw@mail.gmail.com>
 <20240324054636.GT538574@ZenIV>
 <3441a4a1140944f5b418b70f557bca72@huawei.com>
 <20240325-beugen-kraftvoll-1390fd52d59c@brauner>
 <cb267d1c7988460094dbe19d1e7bcece@huawei.com>
 <20240326-halbkreis-wegstecken-8d5886e54d28@brauner>
 <4a0b28ba-be57-4443-b91e-1a744a0feabf@huaweicloud.com>
 <20240328-raushalten-krass-cb040068bde9@brauner>
 <4ad908dc-ddc5-492e-8ed4-d304156b5810@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <4ad908dc-ddc5-492e-8ed4-d304156b5810@huaweicloud.com>

On Thu, Mar 28, 2024 at 01:24:25PM +0200, Roberto Sassu wrote:
> On 3/28/2024 12:08 PM, Christian Brauner wrote:
> > On Thu, Mar 28, 2024 at 12:53:40PM +0200, Roberto Sassu wrote:
> > > On 3/26/2024 12:40 PM, Christian Brauner wrote:
> > > > > we can change the parameter of security_path_post_mknod() from
> > > > > dentry to inode?
> > > > 
> > > > If all current callers only operate on the inode then it seems the best
> > > > to only pass the inode. If there's some reason someone later needs a
> > > > dentry the hook can always be changed.
> > > 
> > > Ok, so the crash is likely caused by:
> > > 
> > > void security_path_post_mknod(struct mnt_idmap *idmap, struct dentry
> > > *dentry)
> > > {
> > >          if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > > 
> > > I guess we can also simply check if there is an inode attached to the
> > > dentry, to minimize the changes. I can do both.
> > > 
> > > More technical question, do I need to do extra checks on the dentry before
> > > calling security_path_post_mknod()?
> > 
> > Why do you need the dentry? The two users I see are ima in [1] and evm in [2].
> > Both of them don't care about the dentry. They only care about the
> > inode. So why is that hook not just:
> 
> Sure, I can definitely do that. Seems an easier fix to do an extra check in
> security_path_post_mknod(), rather than changing the parameter everywhere.

You only have two callers and the generic implementation.

> 
> Next time, when we introduce new LSM hooks we can try to introduce more
> specific parameters.
> 
> Also, consider that the pre hook security_path_mknod() has the dentry as
> parameter. For symmetry, we could keep it in the post hook.

I think that's not that important.

> 
> What I was also asking is if I can still call d_backing_inode() on the
> dentry without extra checks, and avoiding the IS_PRIVATE() check if the
> former returns NULL.