Re: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
On Mon, Oct 5, 2020 at 9:23 PM Andrew Price 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
Re: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
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 --- 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)) /
Re: [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
Hi Fox Chen, On Sat, Oct 3, 2020 at 8:33 AM Fox Chen 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
[PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
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 --- 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