* [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
@ 2020-10-03 6:31 ` Fox Chen
0 siblings, 0 replies; 8+ messages in thread
From: Fox Chen @ 2020-10-03 6:31 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
@ 2020-10-03 6:31 ` Fox Chen
0 siblings, 0 replies; 8+ messages in thread
From: Fox Chen @ 2020-10-03 6:31 UTC (permalink / raw)
To: rpeterso, agruenba; +Cc: Fox Chen, cluster-devel, linux-kernel, gregkh
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
2020-10-03 6:31 ` Fox Chen
@ 2020-10-05 12:59 ` Andreas Gruenbacher
-1 siblings, 0 replies; 8+ messages in thread
From: Andreas Gruenbacher @ 2020-10-05 12:59 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
@ 2020-10-05 12:59 ` Andreas Gruenbacher
0 siblings, 0 replies; 8+ messages in thread
From: Andreas Gruenbacher @ 2020-10-05 12:59 UTC (permalink / raw)
To: Fox Chen; +Cc: Bob Peterson, cluster-devel, LKML, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
2020-10-03 6:31 ` Fox Chen
@ 2020-10-05 13:23 ` Andrew Price
-1 siblings, 0 replies; 8+ messages in thread
From: Andrew Price @ 2020-10-05 13:23 UTC (permalink / raw)
To: cluster-devel.redhat.com
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)) /
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
@ 2020-10-05 13:23 ` Andrew Price
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Price @ 2020-10-05 13:23 UTC (permalink / raw)
To: Fox Chen; +Cc: rpeterso, agruenba, cluster-devel, gregkh, linux-kernel
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)) /
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
2020-10-05 13:23 ` Andrew Price
@ 2020-10-05 14:08 ` Fox Chen
-1 siblings, 0 replies; 8+ messages in thread
From: Fox Chen @ 2020-10-05 14:08 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop
@ 2020-10-05 14:08 ` Fox Chen
0 siblings, 0 replies; 8+ messages in thread
From: Fox Chen @ 2020-10-05 14:08 UTC (permalink / raw)
To: Andrew Price; +Cc: rpeterso, agruenba, cluster-devel, Greg KH, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-10-05 14:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-03 6:31 [Cluster-devel] [PATCH] gfs2: gfs2_read_sb: put gfs2_assert inside the loop Fox Chen
2020-10-03 6:31 ` Fox Chen
2020-10-05 12:59 ` [Cluster-devel] " Andreas Gruenbacher
2020-10-05 12:59 ` Andreas Gruenbacher
2020-10-05 13:23 ` [Cluster-devel] " Andrew Price
2020-10-05 13:23 ` Andrew Price
2020-10-05 14:08 ` Fox Chen
2020-10-05 14:08 ` Fox Chen
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.