linux-kernel - Re: [PATCH v2] fs: modify the annotation of vfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240615065528.GP1629371@ZenIV>
Date: Sat, 15 Jun 2024 07:55:28 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Christoph Hellwig <hch@...radead.org>
Cc: Congjie Zhou <zcjie0802@...com>, brauner@...nel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fs: modify the annotation of vfs_mkdir() in fs/namei.c

On Fri, Jun 14, 2024 at 11:29:38PM -0700, Christoph Hellwig wrote:
> On Sat, Jun 15, 2024 at 12:38:32PM +0800, Congjie Zhou wrote:
> > modify the annotation of @dir and @dentry
> 
> Well, it is clearly obvious that you modify them from the patch.  But
> why?

Look at the current comment:

 * @dir:        inode of @dentry

It is an inode of _some_ dentry; it's most definitely not that
of the argument named 'dentry'.

 * @dentry:     pointer to dentry of the base directory

No.  The first thing vfs_mkdir() does is
        int error = may_create(idmap, dir, dentry);

	if (error)
		return error;

Look at may_create().  Starts with
static inline int may_create(struct mnt_idmap *idmap,
                             struct inode *dir, struct dentry *child)
{
     audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
     if (child->d_inode)
	     return -EEXIST;

If the last argument (aka. 'dentry' argument of vfs_mkdir()) is currently
referring to *ANY* directory, you get -EEXIST.  For a good and simple
reason: it is the dentry of subdirectory to be created.  Now, the second
argument of vfs_mkdir() ('dir') is the inode of the parent to be (or base
directory, if you will).

While we are at it, the rest of comments coming from the same commit
suffer similar problems.  vfs_create(), vfs_symlink(), et.al.
vfs_unlink() is fine, vfs_rmdir() should match vfs_unlink() (inode of
parent + dentry of victim).