[Cluster-devel] [PATCH] libgfs2: Add sd_heightsize bounds checking in read_sb

[Cluster-devel] [PATCH] libgfs2: Add sd_heightsize bounds checking in read_sb

[Andrew Price]

read_sb wasn't checking that x was less than the size of sd_heightsize
when looping over it. This patch adds a check for that. This resolves a
segfault in all tools which use read_sb, when the sb_bsize has been
zeroed in the superblock.

A test case has been added for this scenario in tests/fsck.at

Resolves: bz#1019226

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/libgfs2/super.c | 2 +-
 tests/fsck.at        | 8 ++++++++
 tests/testsuite.at   | 9 +++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 tests/fsck.at

diff --git a/gfs2/libgfs2/super.c b/gfs2/libgfs2/super.c
index f87734a..8ffd144 100644
--- a/gfs2/libgfs2/super.c
+++ b/gfs2/libgfs2/super.c
@@ -88,7 +88,7 @@ int read_sb(struct gfs2_sbd *sdp)
 	sdp->sd_hash_ptrs = sdp->sd_hash_bsize / sizeof(uint64_t);
 	sdp->sd_heightsize[0] = sdp->sd_sb.sb_bsize - sizeof(struct gfs2_dinode);
 	sdp->sd_heightsize[1] = sdp->sd_sb.sb_bsize * sdp->sd_diptrs;
-	for (x = 2; ; x++){
+	for (x = 2; x <= GFS2_MAX_META_HEIGHT; x++){
 		space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
 		/* FIXME: Do we really need this first check?? */
 		if (space / sdp->sd_inptrs != sdp->sd_heightsize[x - 1] ||
diff --git a/tests/fsck.at b/tests/fsck.at
new file mode 100644
index 0000000..34c5bd5
--- /dev/null
+++ b/tests/fsck.at
@@ -0,0 +1,8 @@
+AT_TESTED([fsck.gfs2])
+AT_BANNER([fsck.gfs2 tests])
+
+AT_SETUP([Zeroed block size])
+GFS_LANG_CHECK(
+	[mkfs.gfs2 -O -p lock_nolock $GFS_TGT],
+	[set sb { sb_bsize: 0 }])
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 2c74985..fc90879 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -9,8 +9,17 @@ m4_define([GFS_FSCK_CHECK],
 AT_CHECK($1, 0, [ignore], [ignore])
 AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])])
 
+# Regenerate, mkfs, modify fs with gfs2l, fsck
+m4_define([GFS_LANG_CHECK],
+[GFS_TGT_REGEN
+AT_CHECK($1, 0, [ignore], [ignore])
+AT_CHECK([echo "$2" | gfs2l ${GFS_TGT}], 0, [ignore], [ignore])
+AT_CHECK([fsck.gfs2 -y $GFS_TGT], 1, [ignore], [ignore])
+AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])])
+
 AT_INIT([])
 AT_COLOR_TESTS
 
 m4_include([mkfs.at])
+m4_include([fsck.at])
 m4_include([libgfs2.at])
-- 
1.8.3.1

[Cluster-devel] [PATCH] libgfs2: Add sd_heightsize bounds checking in read_sb

[Andrew Price]

On Mon, Oct 28, 2013 at 04:56:56PM +0000, Andrew Price wrote:
> diff --git a/tests/fsck.at b/tests/fsck.at
> new file mode 100644
> index 0000000..34c5bd5
> --- /dev/null
> +++ b/tests/fsck.at

Oops, forgot to add this new file to the build system. I'll include this
in the final patch:

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3336304..616a44e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -40,6 +40,7 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac
 TESTSUITE_AT = \
        testsuite.at \
        mkfs.at \
+       fsck.at \
        libgfs2.at
 
 TESTSUITE = $(srcdir)/testsuite

[Cluster-devel] [PATCH] libgfs2: Add sd_heightsize bounds checking in read_sb

[Steven Whitehouse]

Hi,

Looks good. thanks,

Steve.

On Mon, 2013-10-28 at 16:56 +0000, Andrew Price wrote:
> read_sb wasn't checking that x was less than the size of sd_heightsize
> when looping over it. This patch adds a check for that. This resolves a
> segfault in all tools which use read_sb, when the sb_bsize has been
> zeroed in the superblock.
> 
> A test case has been added for this scenario in tests/fsck.at
> 
> Resolves: bz#1019226
> 
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  gfs2/libgfs2/super.c | 2 +-
>  tests/fsck.at        | 8 ++++++++
>  tests/testsuite.at   | 9 +++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 tests/fsck.at
> 
> diff --git a/gfs2/libgfs2/super.c b/gfs2/libgfs2/super.c
> index f87734a..8ffd144 100644
> --- a/gfs2/libgfs2/super.c
> +++ b/gfs2/libgfs2/super.c
> @@ -88,7 +88,7 @@ int read_sb(struct gfs2_sbd *sdp)
>  	sdp->sd_hash_ptrs = sdp->sd_hash_bsize / sizeof(uint64_t);
>  	sdp->sd_heightsize[0] = sdp->sd_sb.sb_bsize - sizeof(struct gfs2_dinode);
>  	sdp->sd_heightsize[1] = sdp->sd_sb.sb_bsize * sdp->sd_diptrs;
> -	for (x = 2; ; x++){
> +	for (x = 2; x <= GFS2_MAX_META_HEIGHT; x++){
>  		space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
>  		/* FIXME: Do we really need this first check?? */
>  		if (space / sdp->sd_inptrs != sdp->sd_heightsize[x - 1] ||
> diff --git a/tests/fsck.at b/tests/fsck.at
> new file mode 100644
> index 0000000..34c5bd5
> --- /dev/null
> +++ b/tests/fsck.at
> @@ -0,0 +1,8 @@
> +AT_TESTED([fsck.gfs2])
> +AT_BANNER([fsck.gfs2 tests])
> +
> +AT_SETUP([Zeroed block size])
> +GFS_LANG_CHECK(
> +	[mkfs.gfs2 -O -p lock_nolock $GFS_TGT],
> +	[set sb { sb_bsize: 0 }])
> +AT_CLEANUP
> diff --git a/tests/testsuite.at b/tests/testsuite.at
> index 2c74985..fc90879 100644
> --- a/tests/testsuite.at
> +++ b/tests/testsuite.at
> @@ -9,8 +9,17 @@ m4_define([GFS_FSCK_CHECK],
>  AT_CHECK($1, 0, [ignore], [ignore])
>  AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])])
>  
> +# Regenerate, mkfs, modify fs with gfs2l, fsck
> +m4_define([GFS_LANG_CHECK],
> +[GFS_TGT_REGEN
> +AT_CHECK($1, 0, [ignore], [ignore])
> +AT_CHECK([echo "$2" | gfs2l ${GFS_TGT}], 0, [ignore], [ignore])
> +AT_CHECK([fsck.gfs2 -y $GFS_TGT], 1, [ignore], [ignore])
> +AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])])
> +
>  AT_INIT([])
>  AT_COLOR_TESTS
>  
>  m4_include([mkfs.at])
> +m4_include([fsck.at])
>  m4_include([libgfs2.at])