[PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop

[PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop

[Fox Chen]

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 <foxhlchen@gmail.com>
---
 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

Re: [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop

[Andreas Gruenbacher]

Hi Fox Chen,

On Sat, Oct 3, 2020 at 8:33 AM Fox Chen <foxhlchen@gmail.com> 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

Re: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop

[Andrew Price]

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 <foxhlchen@gmail.com>
> ---
>   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: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop

[Fox Chen]

On Mon, Oct 5, 2020 at 9:23 PM Andrew Price <anprice@redhat.com> 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