[v2] fs: minix: Fix handling of corrupted directories

[PATCH v2] fs: minix: Fix handling of corrupted directories

Posted by Andrey Kriulin 8 months, 4 weeks ago

If the directory is corrupted and the number of nlinks is less than 2
(valid nlinks have at least 2), then when the directory is deleted, the
minix_rmdir will try to reduce the nlinks(unsigned int) to a negative
value.

Make nlinks validity check for directory.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Andrey Kriulin <kitotavrik.media@gmail.com>
Signed-off-by: Andrey Kriulin <kitotavrik.s@gmail.com>
---
v2: Move nlinks validaty check to V[12]_minix_iget() per Jan Kara
<jack@suse.cz> request. Change return error code to EUCLEAN. Don't block
directory in r/o mode per Al Viro <viro@zeniv.linux.org.uk> request.

 fs/minix/inode.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index f007e389d5d2..d815397b8b0d 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -517,6 +517,14 @@ static struct inode *V1_minix_iget(struct inode *inode)
 		iget_failed(inode);
 		return ERR_PTR(-ESTALE);
 	}
+	if (S_ISDIR(raw_inode->i_mode) && raw_inode->i_nlinks < 2) {
+		printk("MINIX-fs: inode directory with corrupted number of links");
+		if (!sb_rdonly(inode->i_sb)) {
+			brelse(bh);
+			iget_failed(inode);
+			return ERR_PTR(-EUCLEAN);
+		}
+	}
 	inode->i_mode = raw_inode->i_mode;
 	i_uid_write(inode, raw_inode->i_uid);
 	i_gid_write(inode, raw_inode->i_gid);
@@ -555,6 +563,14 @@ static struct inode *V2_minix_iget(struct inode *inode)
 		iget_failed(inode);
 		return ERR_PTR(-ESTALE);
 	}
+	if (S_ISDIR(raw_inode->i_mode) && raw_inode->i_nlinks < 2) {
+		printk("MINIX-fs: inode directory with corrupted number of links");
+		if (!sb_rdonly(inode->i_sb)) {
+			brelse(bh);
+			iget_failed(inode);
+			return ERR_PTR(-EUCLEAN);
+		}
+	}
 	inode->i_mode = raw_inode->i_mode;
 	i_uid_write(inode, raw_inode->i_uid);
 	i_gid_write(inode, raw_inode->i_gid);
-- 
2.47.2

Re: [PATCH v2] fs: minix: Fix handling of corrupted directories

Posted by Jan Kara 8 months, 4 weeks ago

On Wed 14-05-25 13:38:35, Andrey Kriulin wrote:
> If the directory is corrupted and the number of nlinks is less than 2
> (valid nlinks have at least 2), then when the directory is deleted, the
> minix_rmdir will try to reduce the nlinks(unsigned int) to a negative
> value.
> 
> Make nlinks validity check for directory.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrey Kriulin <kitotavrik.media@gmail.com>
> Signed-off-by: Andrey Kriulin <kitotavrik.s@gmail.com>
> ---
> v2: Move nlinks validaty check to V[12]_minix_iget() per Jan Kara
> <jack@suse.cz> request. Change return error code to EUCLEAN. Don't block
> directory in r/o mode per Al Viro <viro@zeniv.linux.org.uk> request.
> 
>  fs/minix/inode.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index f007e389d5d2..d815397b8b0d 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -517,6 +517,14 @@ static struct inode *V1_minix_iget(struct inode *inode)
>  		iget_failed(inode);
>  		return ERR_PTR(-ESTALE);
>  	}
> +	if (S_ISDIR(raw_inode->i_mode) && raw_inode->i_nlinks < 2) {
> +		printk("MINIX-fs: inode directory with corrupted number of links");

A message like this is rather useless because it shows nothing either about
the inode or the link count or the filesystem where this happened. I'd
either improve or delete it.

> +		if (!sb_rdonly(inode->i_sb)) {
> +			brelse(bh);
> +			iget_failed(inode);
> +			return ERR_PTR(-EUCLEAN);
> +		}

OK, but when the inode is cached in memory with the wrong link count and
then the filesystem is remounted read-write, you will get the same problem
as before? I guess the easiest is to fudge i_nlinks count in memory to 2 to
avoid issues...


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] fs: minix: Fix handling of corrupted directories

Posted by Kitotavrik 8 months, 4 weeks ago

>  guess the easiest is to fudge i_nlinks count in memory to 2 to
> avoid issues...

But if a subdirectory was in the corrupted directory(nlinks= 3), it
will be replaced with nlinks 2. And after deleting subdirectory,
nlinks was 1 and the problem will remain. Maybe should return EUCLEAN,
regardless of the mounting mode.


ср, 14 мая 2025 г. в 18:54, Jan Kara <jack@suse.cz>:
>
> On Wed 14-05-25 13:38:35, Andrey Kriulin wrote:
> > If the directory is corrupted and the number of nlinks is less than 2
> > (valid nlinks have at least 2), then when the directory is deleted, the
> > minix_rmdir will try to reduce the nlinks(unsigned int) to a negative
> > value.
> >
> > Make nlinks validity check for directory.
> >
> > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Andrey Kriulin <kitotavrik.media@gmail.com>
> > Signed-off-by: Andrey Kriulin <kitotavrik.s@gmail.com>
> > ---
> > v2: Move nlinks validaty check to V[12]_minix_iget() per Jan Kara
> > <jack@suse.cz> request. Change return error code to EUCLEAN. Don't block
> > directory in r/o mode per Al Viro <viro@zeniv.linux.org.uk> request.
> >
> >  fs/minix/inode.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> > index f007e389d5d2..d815397b8b0d 100644
> > --- a/fs/minix/inode.c
> > +++ b/fs/minix/inode.c
> > @@ -517,6 +517,14 @@ static struct inode *V1_minix_iget(struct inode *inode)
> >               iget_failed(inode);
> >               return ERR_PTR(-ESTALE);
> >       }
> > +     if (S_ISDIR(raw_inode->i_mode) && raw_inode->i_nlinks < 2) {
> > +             printk("MINIX-fs: inode directory with corrupted number of links");
>
> A message like this is rather useless because it shows nothing either about
> the inode or the link count or the filesystem where this happened. I'd
> either improve or delete it.
>
> > +             if (!sb_rdonly(inode->i_sb)) {
> > +                     brelse(bh);
> > +                     iget_failed(inode);
> > +                     return ERR_PTR(-EUCLEAN);
> > +             }
>
> OK, but when the inode is cached in memory with the wrong link count and
> then the filesystem is remounted read-write, you will get the same problem
> as before? I guess the easiest is to fudge i_nlinks count in memory to 2 to
> avoid issues...
>
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v2] fs: minix: Fix handling of corrupted directories

Posted by Jan Kara 8 months, 4 weeks ago

On Wed 14-05-25 22:13:24, Kitotavrik wrote:
> >  guess the easiest is to fudge i_nlinks count in memory to 2 to
> > avoid issues...
> 
> But if a subdirectory was in the corrupted directory(nlinks= 3), it
> will be replaced with nlinks 2. And after deleting subdirectory,
> nlinks was 1 and the problem will remain. Maybe should return EUCLEAN,
> regardless of the mounting mode.

Well, that does not help. You can also corrupt the directory link count so
that it is lower than the number of subdirectories. Then as you delete
subdirectories the parent directory link count is going to get invalid. So
at the places where you decrement parent directory link count (rename,
rmdir) you should check whether the link count doesn't drop below expected
minimum and abort the operation (and print error) if it would.

								Honza

> 
> 
> ср, 14 мая 2025 г. в 18:54, Jan Kara <jack@suse.cz>:
> >
> > On Wed 14-05-25 13:38:35, Andrey Kriulin wrote:
> > > If the directory is corrupted and the number of nlinks is less than 2
> > > (valid nlinks have at least 2), then when the directory is deleted, the
> > > minix_rmdir will try to reduce the nlinks(unsigned int) to a negative
> > > value.
> > >
> > > Make nlinks validity check for directory.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Andrey Kriulin <kitotavrik.media@gmail.com>
> > > Signed-off-by: Andrey Kriulin <kitotavrik.s@gmail.com>
> > > ---
> > > v2: Move nlinks validaty check to V[12]_minix_iget() per Jan Kara
> > > <jack@suse.cz> request. Change return error code to EUCLEAN. Don't block
> > > directory in r/o mode per Al Viro <viro@zeniv.linux.org.uk> request.
> > >
> > >  fs/minix/inode.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> > > index f007e389d5d2..d815397b8b0d 100644
> > > --- a/fs/minix/inode.c
> > > +++ b/fs/minix/inode.c
> > > @@ -517,6 +517,14 @@ static struct inode *V1_minix_iget(struct inode *inode)
> > >               iget_failed(inode);
> > >               return ERR_PTR(-ESTALE);
> > >       }
> > > +     if (S_ISDIR(raw_inode->i_mode) && raw_inode->i_nlinks < 2) {
> > > +             printk("MINIX-fs: inode directory with corrupted number of links");
> >
> > A message like this is rather useless because it shows nothing either about
> > the inode or the link count or the filesystem where this happened. I'd
> > either improve or delete it.
> >
> > > +             if (!sb_rdonly(inode->i_sb)) {
> > > +                     brelse(bh);
> > > +                     iget_failed(inode);
> > > +                     return ERR_PTR(-EUCLEAN);
> > > +             }
> >
> > OK, but when the inode is cached in memory with the wrong link count and
> > then the filesystem is remounted read-write, you will get the same problem
> > as before? I guess the easiest is to fudge i_nlinks count in memory to 2 to
> > avoid issues...
> >
> >
> >                                                                 Honza
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR