linux-kernel - Re: [PATCH] ovl: Add check for missing lookup operation on inode

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 for Android: free password hash cracker in your pocket

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

Message-ID: <6fb27fea-3998-0fdf-9210-d7479baf0570@basealt.ru>
Date: Tue, 19 Nov 2024 17:33:03 +0300
From: Vasiliy Kovalev <kovalev@...linux.org>
To: Miklos Szeredi <miklos@...redi.hu>, Amir Goldstein <amir73il@...il.com>
Cc: Vasiliy Kovalev <kovalev@...linux.org>, linux-unionfs@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ovl: Add check for missing lookup operation on inode

Hi,

19.11.2024 12:05, Miklos Szeredi wrote:
> On Mon, 18 Nov 2024 at 19:54, Amir Goldstein <amir73il@...il.com> wrote:
> 
>> Can you analyse what went wrong with the reproducer?
>> How did we get to a state where lowerstack of parent
>> has a dentry which is !d_can_lookup?
> 
> Theoretically we could still get a an S_ISDIR inode, because
> ovl_get_inode() doesn't look at the is_dir value that lookup found.
> I.e. lookup thinks it found a non-dir, but iget will create a dir
> because of the backing inode's type.
> 
> AFAICS this can only happen if i_op->lookup is not set on S_ISDIR for
> the backing inode, which shouldn't happen on normal filesystems.
> Reproducer seems to use bfs, which *should* be normal, and bfs_iget
> certainly doesn't do anything weird in that case, so I still don't
> understand what is happening.

During testing, it was discovered that BFS can return a directory inode 
without a lookup operation.  Adding the following check in bfs_iget:

struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
{

...
	brelse(bh);

+	if (S_ISDIR(inode->i_mode) && !inode->i_op->lookup) {
+		printf("Directory inode missing lookup %s:%08lx\n",
						inode->i_sb->s_id, ino);
+		goto error;
+	}
+
	unlock_new_inode(inode);
	return inode;

error:
	iget_failed(inode);
	return ERR_PTR(-EIO);
}

prevents the error but exposes an invalid inode:

loop0: detected capacity change from 0 to 64
BFS-fs: bfs_iget(): Directory inode missing lookup loop0:00000002
overlayfs: overlapping lowerdir path

Would this be considered a valid workaround, or does BFS require further 
fixes?

> In any case something like the following should filter out such weirdness:
> 
>   bool ovl_dentry_weird(struct dentry *dentry)
>   {
> +       if (!d_can_lookup(dentry) && !d_is_file(dentry) &&
> !d_is_symlink(dentry))
> +               return true;
> +
>          return dentry->d_flags & (DCACHE_NEED_AUTOMOUNT |

I tested this addition successfully.

Additionally, fixes for BFS seem to be discussed reluctantly.
For instance, this patch set [1] has remained unanswered.
Perhaps it would be worth considering discarding invalid inodes at the 
overlayfs level?

[1] 
https://lore.kernel.org/all/20240822161219.459054-1-kovalev@altlinux.org/

> Thanks,
> Miklos
--
Thanks,
Vasiliy Kovalev