Hello,
here are two small fixes that I've spotted in the new support for adding
entries into indexed directories.
Honza
dx_lookup() uses errcode_t return values. As such anything non-zero is
an error, not values less than zero. Fix the error checking to avoid
crashes on corrupted filesystems.
Signed-off-by: Jan Kara <[email protected]>
---
lib/ext2fs/link.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
index 6f523aee718c..7b5bb022117c 100644
--- a/lib/ext2fs/link.c
+++ b/lib/ext2fs/link.c
@@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir,
dx_info.namelen = strlen(name);
again:
retval = dx_lookup(fs, dir, diri, &dx_info);
- if (retval < 0)
+ if (retval)
goto free_buf;
retval = add_dirent_to_buf(fs,
--
2.16.4
There is an off-by-one error in dx_grow_tree() when checking whether we
can add another level to the tree. Thus we can grow tree too much
leading to possible crashes in the library or corrupted filesystem. Fix
the bug.
Signed-off-by: Jan Kara <[email protected]>
---
lib/ext2fs/link.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
index 7b5bb022117c..469eea8cd06d 100644
--- a/lib/ext2fs/link.c
+++ b/lib/ext2fs/link.c
@@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir,
ext2fs_le16_to_cpu(info->frames[i].head->limit))
break;
/* Need to grow tree depth? */
- if (i < 0 && info->levels > ext2_dir_htree_level(fs))
+ if (i < 0 && info->levels >= ext2_dir_htree_level(fs))
return EXT2_ET_DIR_NO_SPACE;
lblk = size / fs->blocksize;
size += fs->blocksize;
--
2.16.4
On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote:
> dx_lookup() uses errcode_t return values. As such anything non-zero is
> an error, not values less than zero. Fix the error checking to avoid
> crashes on corrupted filesystems.
Looks good, thanks.
Signed-off-by: Lukas Czerner <[email protected]>
>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> lib/ext2fs/link.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> index 6f523aee718c..7b5bb022117c 100644
> --- a/lib/ext2fs/link.c
> +++ b/lib/ext2fs/link.c
> @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir,
> dx_info.namelen = strlen(name);
> again:
> retval = dx_lookup(fs, dir, diri, &dx_info);
> - if (retval < 0)
> + if (retval)
> goto free_buf;
>
> retval = add_dirent_to_buf(fs,
> --
> 2.16.4
>
On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote:
> There is an off-by-one error in dx_grow_tree() when checking whether we
> can add another level to the tree. Thus we can grow tree too much
> leading to possible crashes in the library or corrupted filesystem. Fix
> the bug.
Looks good, thanks
Reviewed-by: Lukas Czerner <[email protected]>
Don't we have basically the same off-by-one in
e2fsck/pass1.c handle_htree() ?
if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
-Lukas
>
> Signed-off-by: Jan Kara <[email protected]>
> ---
> lib/ext2fs/link.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> index 7b5bb022117c..469eea8cd06d 100644
> --- a/lib/ext2fs/link.c
> +++ b/lib/ext2fs/link.c
> @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir,
> ext2fs_le16_to_cpu(info->frames[i].head->limit))
> break;
> /* Need to grow tree depth? */
> - if (i < 0 && info->levels > ext2_dir_htree_level(fs))
> + if (i < 0 && info->levels >= ext2_dir_htree_level(fs))
> return EXT2_ET_DIR_NO_SPACE;
> lblk = size / fs->blocksize;
> size += fs->blocksize;
> --
> 2.16.4
>
On Mon, Mar 30, 2020 at 03:24:40PM +0200, Lukas Czerner wrote:
> On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote:
> > dx_lookup() uses errcode_t return values. As such anything non-zero is
> > an error, not values less than zero. Fix the error checking to avoid
> > crashes on corrupted filesystems.
>
> Looks good, thanks.
>
> Signed-off-by: Lukas Czerner <[email protected]>
of course that should be
Reviewed-by: Lukas Czerner <[email protected]>
-Lukas
>
> >
> > Signed-off-by: Jan Kara <[email protected]>
> > ---
> > lib/ext2fs/link.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > index 6f523aee718c..7b5bb022117c 100644
> > --- a/lib/ext2fs/link.c
> > +++ b/lib/ext2fs/link.c
> > @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir,
> > dx_info.namelen = strlen(name);
> > again:
> > retval = dx_lookup(fs, dir, diri, &dx_info);
> > - if (retval < 0)
> > + if (retval)
> > goto free_buf;
> >
> > retval = add_dirent_to_buf(fs,
> > --
> > 2.16.4
> >
>
On Mon 30-03-20 15:27:12, Lukas Czerner wrote:
> On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote:
> > There is an off-by-one error in dx_grow_tree() when checking whether we
> > can add another level to the tree. Thus we can grow tree too much
> > leading to possible crashes in the library or corrupted filesystem. Fix
> > the bug.
>
> Looks good, thanks
>
> Reviewed-by: Lukas Czerner <[email protected]>
>
> Don't we have basically the same off-by-one in
> e2fsck/pass1.c handle_htree() ?
>
> if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
I don't think so. It is indeed correct for root->indirect_levels to be
equal to ext2_dir_htree_level(). However dx_grow_tree() is going to
increase root->indirect_levels by 1 which is where tree would become
invalid...
Honza
> >
> > Signed-off-by: Jan Kara <[email protected]>
> > ---
> > lib/ext2fs/link.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > index 7b5bb022117c..469eea8cd06d 100644
> > --- a/lib/ext2fs/link.c
> > +++ b/lib/ext2fs/link.c
> > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir,
> > ext2fs_le16_to_cpu(info->frames[i].head->limit))
> > break;
> > /* Need to grow tree depth? */
> > - if (i < 0 && info->levels > ext2_dir_htree_level(fs))
> > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs))
> > return EXT2_ET_DIR_NO_SPACE;
> > lblk = size / fs->blocksize;
> > size += fs->blocksize;
> > --
> > 2.16.4
> >
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon 30-03-20 15:48:53, Lukas Czerner wrote:
> On Mon, Mar 30, 2020 at 03:24:40PM +0200, Lukas Czerner wrote:
> > On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote:
> > > dx_lookup() uses errcode_t return values. As such anything non-zero is
> > > an error, not values less than zero. Fix the error checking to avoid
> > > crashes on corrupted filesystems.
> >
> > Looks good, thanks.
> >
> > Signed-off-by: Lukas Czerner <[email protected]>
>
> of course that should be
>
> Reviewed-by: Lukas Czerner <[email protected]>
Thanks for review!
Honza
>
> -Lukas
>
>
> >
> > >
> > > Signed-off-by: Jan Kara <[email protected]>
> > > ---
> > > lib/ext2fs/link.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > > index 6f523aee718c..7b5bb022117c 100644
> > > --- a/lib/ext2fs/link.c
> > > +++ b/lib/ext2fs/link.c
> > > @@ -571,7 +571,7 @@ static errcode_t dx_link(ext2_filsys fs, ext2_ino_t dir,
> > > dx_info.namelen = strlen(name);
> > > again:
> > > retval = dx_lookup(fs, dir, diri, &dx_info);
> > > - if (retval < 0)
> > > + if (retval)
> > > goto free_buf;
> > >
> > > retval = add_dirent_to_buf(fs,
> > > --
> > > 2.16.4
> > >
> >
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Mar 30, 2020 at 04:55:31PM +0200, Jan Kara wrote:
> On Mon 30-03-20 15:27:12, Lukas Czerner wrote:
> > On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote:
> > > There is an off-by-one error in dx_grow_tree() when checking whether we
> > > can add another level to the tree. Thus we can grow tree too much
> > > leading to possible crashes in the library or corrupted filesystem. Fix
> > > the bug.
> >
> > Looks good, thanks
> >
> > Reviewed-by: Lukas Czerner <[email protected]>
> >
> > Don't we have basically the same off-by-one in
> > e2fsck/pass1.c handle_htree() ?
> >
> > if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
>
> I don't think so. It is indeed correct for root->indirect_levels to be
> equal to ext2_dir_htree_level(). However dx_grow_tree() is going to
> increase root->indirect_levels by 1 which is where tree would become
> invalid...
>
> Honza
Hmm, are you sure ? I think the names are very confusing
root->indirect_levels is zero based, while ext2_dir_htree_level()
returns the maximum number of levels (that is 3 by default). If I am
right then indirect_levels must always be smaller then
ext2_dir_htree_level() and that is how we use it everywhere else - the
palce I am pointing out is an exception and I think it's a bug.
Indeed it looks like the bug got introduced in
3f0cf647539970474be8f607017ca7eccfc2fbbe
- if ((root->indirect_levels > 1) &&
+ if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
Or am I missing something ?
-Lukas
>
> > >
> > > Signed-off-by: Jan Kara <[email protected]>
> > > ---
> > > lib/ext2fs/link.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > > index 7b5bb022117c..469eea8cd06d 100644
> > > --- a/lib/ext2fs/link.c
> > > +++ b/lib/ext2fs/link.c
> > > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir,
> > > ext2fs_le16_to_cpu(info->frames[i].head->limit))
> > > break;
> > > /* Need to grow tree depth? */
> > > - if (i < 0 && info->levels > ext2_dir_htree_level(fs))
> > > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs))
> > > return EXT2_ET_DIR_NO_SPACE;
> > > lblk = size / fs->blocksize;
> > > size += fs->blocksize;
> > > --
> > > 2.16.4
> > >
> >
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
>
On Tue 31-03-20 13:33:03, Lukas Czerner wrote:
> On Mon, Mar 30, 2020 at 04:55:31PM +0200, Jan Kara wrote:
> > On Mon 30-03-20 15:27:12, Lukas Czerner wrote:
> > > On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote:
> > > > There is an off-by-one error in dx_grow_tree() when checking whether we
> > > > can add another level to the tree. Thus we can grow tree too much
> > > > leading to possible crashes in the library or corrupted filesystem. Fix
> > > > the bug.
> > >
> > > Looks good, thanks
> > >
> > > Reviewed-by: Lukas Czerner <[email protected]>
> > >
> > > Don't we have basically the same off-by-one in
> > > e2fsck/pass1.c handle_htree() ?
> > >
> > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
> >
> > I don't think so. It is indeed correct for root->indirect_levels to be
> > equal to ext2_dir_htree_level(). However dx_grow_tree() is going to
> > increase root->indirect_levels by 1 which is where tree would become
> > invalid...
> >
> > Honza
>
> Hmm, are you sure ? I think the names are very confusing
>
> root->indirect_levels is zero based, while ext2_dir_htree_level()
> returns the maximum number of levels (that is 3 by default). If I am
> right then indirect_levels must always be smaller then
> ext2_dir_htree_level() and that is how we use it everywhere else - the
> palce I am pointing out is an exception and I think it's a bug.
>
> Indeed it looks like the bug got introduced in
> 3f0cf647539970474be8f607017ca7eccfc2fbbe
>
> - if ((root->indirect_levels > 1) &&
> + if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
>
> Or am I missing something ?
Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the
condition. Just the condition in pass1.c is wrong.
Honza
>
> -Lukas
>
> >
> > > >
> > > > Signed-off-by: Jan Kara <[email protected]>
> > > > ---
> > > > lib/ext2fs/link.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > > > index 7b5bb022117c..469eea8cd06d 100644
> > > > --- a/lib/ext2fs/link.c
> > > > +++ b/lib/ext2fs/link.c
> > > > @@ -473,7 +473,7 @@ static errcode_t dx_grow_tree(ext2_filsys fs, ext2_ino_t dir,
> > > > ext2fs_le16_to_cpu(info->frames[i].head->limit))
> > > > break;
> > > > /* Need to grow tree depth? */
> > > > - if (i < 0 && info->levels > ext2_dir_htree_level(fs))
> > > > + if (i < 0 && info->levels >= ext2_dir_htree_level(fs))
> > > > return EXT2_ET_DIR_NO_SPACE;
> > > > lblk = size / fs->blocksize;
> > > > size += fs->blocksize;
> > > > --
> > > > 2.16.4
> > > >
> > >
> > --
> > Jan Kara <[email protected]>
> > SUSE Labs, CR
> >
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Mar 30, 2020 at 11:09:31AM +0200, Jan Kara wrote:
> dx_lookup() uses errcode_t return values. As such anything non-zero is
> an error, not values less than zero. Fix the error checking to avoid
> crashes on corrupted filesystems.
>
> Signed-off-by: Jan Kara <[email protected]>
Applied, thanks.
- Ted
On Mon, Mar 30, 2020 at 11:09:32AM +0200, Jan Kara wrote:
> There is an off-by-one error in dx_grow_tree() when checking whether we
> can add another level to the tree. Thus we can grow tree too much
> leading to possible crashes in the library or corrupted filesystem. Fix
> the bug.
>
> Signed-off-by: Jan Kara <[email protected]>
Applied, thanks.
- Ted
On Tue, Mar 31, 2020 at 04:30:35PM +0200, Jan Kara wrote:
> > > > Don't we have basically the same off-by-one in
> > > > e2fsck/pass1.c handle_htree() ?
> > > >
> > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> > > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
> > >
> >
> > root->indirect_levels is zero based, while ext2_dir_htree_level()
> > returns the maximum number of levels (that is 3 by default). If I am
> > right then indirect_levels must always be smaller then
> > ext2_dir_htree_level() and that is how we use it everywhere else - the
> > palce I am pointing out is an exception and I think it's a bug.
> >
> > Indeed it looks like the bug got introduced in
> > 3f0cf647539970474be8f607017ca7eccfc2fbbe
> >
> > - if ((root->indirect_levels > 1) &&
> > + if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> >
> > Or am I missing something ?
>
> Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the
> condition. Just the condition in pass1.c is wrong.
I've applied the following fix on the maint branch.
- Ted
commit 759b387775bfd5c9d3692680e5e4b929c3848d51
Author: Theodore Ts'o <[email protected]>
Date: Fri Apr 10 00:30:52 2020 -0400
e2fsck: fix off-by-one check when validating depth of an htree
Fixes: 3f0cf6475399 ("e2fsprogs: add support for 3-level htree")
Signed-off-by: Theodore Ts'o <[email protected]>
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index c9e8bf82..38afda48 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -2685,7 +2685,7 @@ static int handle_htree(e2fsck_t ctx, struct problem_context *pctx,
return 1;
pctx->num = root->indirect_levels;
- if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
+ if ((root->indirect_levels >= ext2_dir_htree_level(fs)) &&
fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
return 1;
On Fri 10-04-20 00:33:21, Theodore Y. Ts'o wrote:
> On Tue, Mar 31, 2020 at 04:30:35PM +0200, Jan Kara wrote:
> > > > > Don't we have basically the same off-by-one in
> > > > > e2fsck/pass1.c handle_htree() ?
> > > > >
> > > > > if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> > > > > fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
> > > >
> > >
> > > root->indirect_levels is zero based, while ext2_dir_htree_level()
> > > returns the maximum number of levels (that is 3 by default). If I am
> > > right then indirect_levels must always be smaller then
> > > ext2_dir_htree_level() and that is how we use it everywhere else - the
> > > palce I am pointing out is an exception and I think it's a bug.
> > >
> > > Indeed it looks like the bug got introduced in
> > > 3f0cf647539970474be8f607017ca7eccfc2fbbe
> > >
> > > - if ((root->indirect_levels > 1) &&
> > > + if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> > >
> > > Or am I missing something ?
> >
> > Ah, you're indeed right! e2fsck/pass2.c even has a correct version of the
> > condition. Just the condition in pass1.c is wrong.
>
> I've applied the following fix on the maint branch.
>
> - Ted
>
> commit 759b387775bfd5c9d3692680e5e4b929c3848d51
> Author: Theodore Ts'o <[email protected]>
> Date: Fri Apr 10 00:30:52 2020 -0400
>
> e2fsck: fix off-by-one check when validating depth of an htree
>
> Fixes: 3f0cf6475399 ("e2fsprogs: add support for 3-level htree")
>
> Signed-off-by: Theodore Ts'o <[email protected]>
Cool, thanks! Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
Honza
>
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index c9e8bf82..38afda48 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2685,7 +2685,7 @@ static int handle_htree(e2fsck_t ctx, struct problem_context *pctx,
> return 1;
>
> pctx->num = root->indirect_levels;
> - if ((root->indirect_levels > ext2_dir_htree_level(fs)) &&
> + if ((root->indirect_levels >= ext2_dir_htree_level(fs)) &&
> fix_problem(ctx, PR_1_HTREE_DEPTH, pctx))
> return 1;
>
--
Jan Kara <[email protected]>
SUSE Labs, CR