[PATCH] ovl: Add check for missing lookup operation on inode

[PATCH] ovl: Add check for missing lookup operation on inode

[Vasiliy Kovalev]

Ensure that the lookup operation is present for the inode in the overlay
filesystem. If the operation is missing, log a warning and return an EIO
error to prevent further issues in the lookup process.

Reported-by: syzbot+a8c9d476508bd14a90e5@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=a8c9d476508bd14a90e5
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 fs/overlayfs/namei.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5764f91d283e7..a73f37e401cf0 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1115,6 +1115,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
 		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
+		if (!lower.dentry->d_inode->i_op->lookup) {
+			err = -EIO;
+			pr_warn_ratelimited("missing lookup operation for inode %p\n",
+								lower.dentry->d_inode);
+			goto out_put;
+		}
+
 		if (!ovl_redirect_follow(ofs))
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
-- 
2.33.8

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

[Amir Goldstein]

On Mon, Nov 18, 2024 at 3:17 PM Vasiliy Kovalev <kovalev@altlinux.org> wrote:
>
> Ensure that the lookup operation is present for the inode in the overlay
> filesystem. If the operation is missing, log a warning and return an EIO
> error to prevent further issues in the lookup process.
>
> Reported-by: syzbot+a8c9d476508bd14a90e5@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=a8c9d476508bd14a90e5
> Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
> ---
>  fs/overlayfs/namei.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5764f91d283e7..a73f37e401cf0 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1115,6 +1115,13 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
>                 struct ovl_path lower = ovl_lowerstack(poe)[i];
>
> +               if (!lower.dentry->d_inode->i_op->lookup) {
> +                       err = -EIO;
> +                       pr_warn_ratelimited("missing lookup operation for inode %p\n",
> +                                                               lower.dentry->d_inode);
> +                       goto out_put;
> +               }
> +

This looks like it is papering over a bug.
The dentries in the poe lower stack are supposed to be
d_can_lookup(), which means that they should have a ->lookup op.

See in ovl_lookup_single():
         if (!d_can_lookup(this)) {
                if (d->is_dir || !last_element) {
                        d->stop = true;
                        goto put_and_out;
                }

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?

Thanks,
Amir.

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

[Miklos Szeredi]

On Mon, 18 Nov 2024 at 19:54, Amir Goldstein <amir73il@gmail.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.

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 |

Thanks,
Miklos

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

[Vasiliy Kovalev]

Hi,

19.11.2024 12:05, Miklos Szeredi wrote:
> On Mon, 18 Nov 2024 at 19:54, Amir Goldstein <amir73il@gmail.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

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

[Amir Goldstein]

On Tue, Nov 19, 2024 at 3:33 PM Vasiliy Kovalev <kovalev@altlinux.org> wrote:
>
> Hi,
>
> 19.11.2024 12:05, Miklos Szeredi wrote:
> > On Mon, 18 Nov 2024 at 19:54, Amir Goldstein <amir73il@gmail.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?

Sure. please send proper patch following Miklos' suggestion

Thanks,
Amir.

[PATCH v2] ovl: Filter invalid inodes with missing lookup function

[Vasiliy Kovalev]

Add a check to the ovl_dentry_weird() function to prevent the
processing of directory inodes that lack the lookup function.
This is important because such inodes can cause errors in overlayfs
when passed to the lowerstack.

Reported-by: syzbot+a8c9d476508bd14a90e5@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=a8c9d476508bd14a90e5
Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
Cc: <stable@vger.kernel.org>
---
 fs/overlayfs/util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3bb107471fb42..9aa7493b1e103 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -202,6 +202,9 @@ void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
 
 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 |
 				  DCACHE_MANAGE_TRANSIT |
 				  DCACHE_OP_HASH |
-- 
2.33.8

Re: [PATCH v2] ovl: Filter invalid inodes with missing lookup function

[Amir Goldstein]

On Tue, Nov 19, 2024 at 4:58 PM Vasiliy Kovalev <kovalev@altlinux.org> wrote:
>
> Add a check to the ovl_dentry_weird() function to prevent the
> processing of directory inodes that lack the lookup function.
> This is important because such inodes can cause errors in overlayfs
> when passed to the lowerstack.
>
> Reported-by: syzbot+a8c9d476508bd14a90e5@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=a8c9d476508bd14a90e5
> Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
> Cc: <stable@vger.kernel.org>
> ---
>  fs/overlayfs/util.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 3bb107471fb42..9aa7493b1e103 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -202,6 +202,9 @@ void ovl_dentry_init_flags(struct dentry *dentry, struct dentry *upperdentry,
>
>  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 |
>                                   DCACHE_MANAGE_TRANSIT |
>                                   DCACHE_OP_HASH |
> --
> 2.33.8
>

Applied to overlayfs-next. Will send along with 6.13 PR

Thanks,
Amir.

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

[Al Viro]

On Tue, Nov 19, 2024 at 05:33:03PM +0300, Vasiliy Kovalev wrote:

> 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?

Yes, it does.  Note that this
        inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
sets the bits 0..15, which includes not only the permissions
(0..11), but the file type as well.  And those |= are not
going to be enough to prevent trouble - what we have there
is
	0x1000 => FIFO
	0x2000 => CHR
	0x4000 => DIR
	0x6000 => BLK
	0x8000 => REG
	0xa000 => LNK
	0xe000 => SOCK

So depending upon ->i_vtype you get one of
	* ->i_op and ->i_fop set for directory, type bits - 0x4000 | junk
	* ->i_op and ->i_fop set for regular file, type bits - 0x8000 | junk
	* ->i_op and ->i_fop left empty, type bits - junk.

Frankly, I would rather ignore bits 12..15 (i.e.
        inode->i_mode = 0x00000FFF & le32_to_cpu(di->i_mode);
instead of
        inode->i_mode = 0x0000FFFF & le32_to_cpu(di->i_mode);
) and complain (and fail) if ->i_vtype value is fucked up.