for (x = 2;; x++) {
...
gfs2_assert(sdp, x <= GFS2_MAX_META_HEIGHT); <--- after
...
if (d != sdp->sd_heightsize[x - 1] || m)
break;
sdp->sd_heightsize[x] = space;
}
sdp->sd_max_height = x
gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT) <--- before
Before this patch, gfs2_assert is put outside of the loop of
sdp->sd_heightsize[x] calculation. When something goes wrong,
x exceeds the size of GFS2_MAX_META_HEIGHT, it may already crash inside
the loop when
sdp->sd_heightsize[x] = space
tries to reach the out-of-bound
location, gfs2_assert won't help here.
This patch fixes this by moving gfs2_assert into the loop.
We will check x value each time to see if it exceeds GFS2_MAX_META_HEIGHT.
Signed-off-by: Fox Chen <[email protected]>
---
fs/gfs2/ops_fstype.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 6d18d2c91add..6cc32e3010f2 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -333,6 +333,7 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
u64 space, d;
u32 m;
+ gfs2_assert(sdp, x <= GFS2_MAX_META_HEIGHT);
space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
d = space;
m = do_div(d, sdp->sd_inptrs);
@@ -343,7 +344,6 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
}
sdp->sd_max_height = x;
sdp->sd_heightsize[x] = ~0;
- gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT);
sdp->sd_max_dents_per_leaf = (sdp->sd_sb.sb_bsize -
sizeof(struct gfs2_leaf)) /
--
2.25.1
Hi Fox Chen,
On Sat, Oct 3, 2020 at 8:33 AM Fox Chen <[email protected]> wrote:
> Before this patch, gfs2_assert is put outside of the loop of
> sdp->sd_heightsize[x] calculation. When something goes wrong,
> x exceeds the size of GFS2_MAX_META_HEIGHT, it may already crash inside
> the loop when
>
> sdp->sd_heightsize[x] = space
>
> tries to reach the out-of-bound
> location, gfs2_assert won't help here.
that's true, but the smallest possible block size is 1024 bytes, and
with that, the height cannot grow bigger than 10. So the assert is
basically there only for documentation purposes.
Thanks,
Andreas
On 03/10/2020 07:31, Fox Chen wrote:
> for (x = 2;; x++) {
> ...
> gfs2_assert(sdp, x <= GFS2_MAX_META_HEIGHT); <--- after
> ...
> if (d != sdp->sd_heightsize[x - 1] || m)
> break;
> sdp->sd_heightsize[x] = space;
> }
>
> sdp->sd_max_height = x
> gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT) <--- before
>
> Before this patch, gfs2_assert is put outside of the loop of
> sdp->sd_heightsize[x] calculation. When something goes wrong,
So this looks related to one of the recent syzbot reports, where the
"something goes wrong" is the block size in the on-disk superblock was
zeroed and that leads eventually to this out-of-bounds write. The
correct fix in that case would be to add a validity check for the block
size in gfs2_check_sb().
Andy
> x exceeds the size of GFS2_MAX_META_HEIGHT, it may already crash inside
> the loop when
>
> sdp->sd_heightsize[x] = space
>
> tries to reach the out-of-bound
> location, gfs2_assert won't help here.
>
> This patch fixes this by moving gfs2_assert into the loop.
> We will check x value each time to see if it exceeds GFS2_MAX_META_HEIGHT.
>
> Signed-off-by: Fox Chen <[email protected]>
> ---
> fs/gfs2/ops_fstype.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 6d18d2c91add..6cc32e3010f2 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -333,6 +333,7 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
> u64 space, d;
> u32 m;
>
> + gfs2_assert(sdp, x <= GFS2_MAX_META_HEIGHT);
> space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
> d = space;
> m = do_div(d, sdp->sd_inptrs);
> @@ -343,7 +344,6 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
> }
> sdp->sd_max_height = x;
> sdp->sd_heightsize[x] = ~0;
> - gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT);
>
> sdp->sd_max_dents_per_leaf = (sdp->sd_sb.sb_bsize -
> sizeof(struct gfs2_leaf)) /
>
On Mon, Oct 5, 2020 at 9:23 PM Andrew Price <[email protected]> wrote:
>
> On 03/10/2020 07:31, Fox Chen wrote:
> > for (x = 2;; x++) {
> > ...
> > gfs2_assert(sdp, x <= GFS2_MAX_META_HEIGHT); <--- after
> > ...
> > if (d != sdp->sd_heightsize[x - 1] || m)
> > break;
> > sdp->sd_heightsize[x] = space;
> > }
> >
> > sdp->sd_max_height = x
> > gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT) <--- before
> >
> > Before this patch, gfs2_assert is put outside of the loop of
> > sdp->sd_heightsize[x] calculation. When something goes wrong,
>
> So this looks related to one of the recent syzbot reports, where the
> "something goes wrong" is the block size in the on-disk superblock was
> zeroed and that leads eventually to this out-of-bounds write. The
> correct fix in that case would be to add a validity check for the block
> size in gfs2_check_sb().
>
Yes, I saw this bug from the syzbot report and I though instead of
KASAN gfs2_assert should be able to catch it so I proposed this patch.
:)
thank you both for your comments.
fox