* [PATCH 0/2] Fixes for dir_index support in libext2fs
@ 2020-03-30 9:09 Jan Kara
2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara
2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara
0 siblings, 2 replies; 14+ messages in thread
From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara
Hello,
here are two small fixes that I've spotted in the new support for adding
entries into indexed directories.
Honza
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] ext2fs: Fix error checking in dx_link()
2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara
@ 2020-03-30 9:09 ` Jan Kara
2020-03-30 13:24 ` Lukas Czerner
2020-04-10 3:52 ` Theodore Y. Ts'o
2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara
1 sibling, 2 replies; 14+ messages in thread
From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara
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 <jack@suse.cz>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara
2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara
@ 2020-03-30 9:09 ` Jan Kara
2020-03-30 13:27 ` Lukas Czerner
2020-04-10 4:32 ` Theodore Y. Ts'o
1 sibling, 2 replies; 14+ messages in thread
From: Jan Kara @ 2020-03-30 9:09 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara
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 <jack@suse.cz>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link()
2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara
@ 2020-03-30 13:24 ` Lukas Czerner
2020-03-30 13:48 ` Lukas Czerner
2020-04-10 3:52 ` Theodore Y. Ts'o
1 sibling, 1 reply; 14+ messages in thread
From: Lukas Czerner @ 2020-03-30 13:24 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4
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 <lczerner@redhat.com>
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara
@ 2020-03-30 13:27 ` Lukas Czerner
2020-03-30 14:55 ` Jan Kara
2020-04-10 4:32 ` Theodore Y. Ts'o
1 sibling, 1 reply; 14+ messages in thread
From: Lukas Czerner @ 2020-03-30 13:27 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4
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 <lczerner@redhat.com>
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 <jack@suse.cz>
> ---
> 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link()
2020-03-30 13:24 ` Lukas Czerner
@ 2020-03-30 13:48 ` Lukas Czerner
2020-03-30 14:55 ` Jan Kara
0 siblings, 1 reply; 14+ messages in thread
From: Lukas Czerner @ 2020-03-30 13:48 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4
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 <lczerner@redhat.com>
of course that should be
Reviewed-by: Lukas Czerner <lczerner@redhat.com>
-Lukas
>
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > 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
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-30 13:27 ` Lukas Czerner
@ 2020-03-30 14:55 ` Jan Kara
2020-03-31 11:33 ` Lukas Czerner
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kara @ 2020-03-30 14:55 UTC (permalink / raw)
To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4
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 <lczerner@redhat.com>
>
> 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 <jack@suse.cz>
> > ---
> > 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 <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link()
2020-03-30 13:48 ` Lukas Czerner
@ 2020-03-30 14:55 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2020-03-30 14:55 UTC (permalink / raw)
To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4
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 <lczerner@redhat.com>
>
> of course that should be
>
> Reviewed-by: Lukas Czerner <lczerner@redhat.com>
Thanks for review!
Honza
>
> -Lukas
>
>
> >
> > >
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > > 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 <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-30 14:55 ` Jan Kara
@ 2020-03-31 11:33 ` Lukas Czerner
2020-03-31 14:30 ` Jan Kara
0 siblings, 1 reply; 14+ messages in thread
From: Lukas Czerner @ 2020-03-31 11:33 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4
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 <lczerner@redhat.com>
> >
> > 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 <jack@suse.cz>
> > > ---
> > > 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 <jack@suse.com>
> SUSE Labs, CR
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-31 11:33 ` Lukas Czerner
@ 2020-03-31 14:30 ` Jan Kara
2020-04-10 4:33 ` Theodore Y. Ts'o
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kara @ 2020-03-31 14:30 UTC (permalink / raw)
To: Lukas Czerner; +Cc: Jan Kara, Ted Tso, linux-ext4
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 <lczerner@redhat.com>
> > >
> > > 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 <jack@suse.cz>
> > > > ---
> > > > 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 <jack@suse.com>
> > SUSE Labs, CR
> >
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ext2fs: Fix error checking in dx_link()
2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara
2020-03-30 13:24 ` Lukas Czerner
@ 2020-04-10 3:52 ` Theodore Y. Ts'o
1 sibling, 0 replies; 14+ messages in thread
From: Theodore Y. Ts'o @ 2020-04-10 3:52 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4
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 <jack@suse.cz>
Applied, thanks.
- Ted
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara
2020-03-30 13:27 ` Lukas Czerner
@ 2020-04-10 4:32 ` Theodore Y. Ts'o
1 sibling, 0 replies; 14+ messages in thread
From: Theodore Y. Ts'o @ 2020-04-10 4:32 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4
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 <jack@suse.cz>
Applied, thanks.
- Ted
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-03-31 14:30 ` Jan Kara
@ 2020-04-10 4:33 ` Theodore Y. Ts'o
2020-04-10 15:09 ` Jan Kara
0 siblings, 1 reply; 14+ messages in thread
From: Theodore Y. Ts'o @ 2020-04-10 4:33 UTC (permalink / raw)
To: Jan Kara; +Cc: Lukas Czerner, linux-ext4
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 <tytso@mit.edu>
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 <tytso@mit.edu>
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;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree()
2020-04-10 4:33 ` Theodore Y. Ts'o
@ 2020-04-10 15:09 ` Jan Kara
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kara @ 2020-04-10 15:09 UTC (permalink / raw)
To: Theodore Y. Ts'o; +Cc: Jan Kara, Lukas Czerner, linux-ext4
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 <tytso@mit.edu>
> 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 <tytso@mit.edu>
Cool, thanks! Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
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 <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-04-10 15:09 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 9:09 [PATCH 0/2] Fixes for dir_index support in libext2fs Jan Kara
2020-03-30 9:09 ` [PATCH 1/2] ext2fs: Fix error checking in dx_link() Jan Kara
2020-03-30 13:24 ` Lukas Czerner
2020-03-30 13:48 ` Lukas Czerner
2020-03-30 14:55 ` Jan Kara
2020-04-10 3:52 ` Theodore Y. Ts'o
2020-03-30 9:09 ` [PATCH 2/2] ext2fs: Fix off-by-one in dx_grow_tree() Jan Kara
2020-03-30 13:27 ` Lukas Czerner
2020-03-30 14:55 ` Jan Kara
2020-03-31 11:33 ` Lukas Czerner
2020-03-31 14:30 ` Jan Kara
2020-04-10 4:33 ` Theodore Y. Ts'o
2020-04-10 15:09 ` Jan Kara
2020-04-10 4:32 ` Theodore Y. Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.