Re: [PATCH 1/1] nilfs2: fix potential memory overrun on inode
On Fri, 20 Feb 2015 18:00:55 -0800, Andrew Morton wrote: > On Sat, 21 Feb 2015 10:13:28 +0900 (JST) Ryusuke Konishi > wrote: > >> I've got a warning from 0day kernel testing backend: >> >> fs/nilfs2/btree.c: In function 'nilfs_btree_root_broken': >> >> fs/nilfs2/btree.c:394:3: warning: format '%lu' expects argument of type >> >> 'long unsigned int', but argument 2 has type 'ino_t' [-Wformat=] >>pr_crit("NILFS: bad btree root (inode number=%lu): level = %d, >> flags = 0x%x, nchildren = %d\n", >>^ >> >> This is output for s390 arch since ino_t doesn't mean "unsigned long" >> in s390. > > alpha uses uint for ino_t as well. > > It seems a bit pointless - neither arch uses ino_t in ./arch/ code. I > suspect both could switch to ulong, which would make the world a > slightly better place. I entirely agree. Regards, Ryusuke Konishi -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] nilfs2: fix potential memory overrun on inode
On Sat, 21 Feb 2015 10:13:28 +0900 (JST) Ryusuke Konishi wrote: > I've got a warning from 0day kernel testing backend: > > fs/nilfs2/btree.c: In function 'nilfs_btree_root_broken': > >> fs/nilfs2/btree.c:394:3: warning: format '%lu' expects argument of type > >> 'long unsigned int', but argument 2 has type 'ino_t' [-Wformat=] >pr_crit("NILFS: bad btree root (inode number=%lu): level = %d, > flags = 0x%x, nchildren = %d\n", >^ > > This is output for s390 arch since ino_t doesn't mean "unsigned long" > in s390. alpha uses uint for ino_t as well. It seems a bit pointless - neither arch uses ino_t in ./arch/ code. I suspect both could switch to ulong, which would make the world a slightly better place. -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] nilfs2: fix potential memory overrun on inode
On Sat, 21 Feb 2015 09:22:08 +0900 (JST), Ryusuke Konishi wrote: > On Fri, 20 Feb 2015 13:58:42 -0800, Andrew Morton wrote: >> On Fri, 20 Feb 2015 22:46:35 +0900 Ryusuke Konishi >> wrote: >> >>> Each inode of nilfs2 stores a root node of a b-tree, and it turned out >>> to have a memory overrun issue: >>> >>> Each b-tree node of nilfs2 stores a set of key-value pairs and the >>> number of them (in "bn_nchildren" member of nilfs_btree_node struct), >>> as well as a few other "bn_*" members. >>> >>> Since the value of "bn_nchildren" is used for operations on the >>> key-values within the b-tree node, it can cause memory access overrun >>> if a large number is incorrectly set to "bn_nchildren". >>> >>> For instance, nilfs_btree_node_lookup() function determines the range >>> of binary search with it, and too large "bn_nchildren" leads >>> nilfs_btree_node_get_key() in that function to overrun. >>> >>> As for intermediate b-tree nodes, this is prevented by a sanity check >>> performed when each node is read from a drive, however, no sanity >>> check has been done for root nodes stored in inodes. >>> >>> This patch fixes the issue by adding missing sanity check against >>> b-tree root nodes so that it's called when on-memory inodes are read >>> from ifile, inode metadata file. >> >> How would one trigger this overrun? Mount an fs with a deliberately >> corrupted/inconsistent fs image? > > Yes, this can be triggered by mounting an fs with a corrupted image > deliberately or by chance. > >> Memory overrun sounds nasty so I'm thinking we add cc:stable to this >> one. OK? > > Agreed. Could you apply the following amendment ? I've got a warning from 0day kernel testing backend: fs/nilfs2/btree.c: In function 'nilfs_btree_root_broken': >> fs/nilfs2/btree.c:394:3: warning: format '%lu' expects argument of type >> 'long unsigned int', but argument 2 has type 'ino_t' [-Wformat=] pr_crit("NILFS: bad btree root (inode number=%lu): level = %d, flags = 0x%x, nchildren = %d\n", ^ This is output for s390 arch since ino_t doesn't mean "unsigned long" in s390. Thanks, Ryusuke Konishi -- diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c index c645d7c..ecdbae1 100644 --- a/fs/nilfs2/btree.c +++ b/fs/nilfs2/btree.c @@ -378,7 +378,7 @@ static int nilfs_btree_node_broken(const struct nilfs_btree_node *node, * Return Value: If node is broken, 1 is returned. Otherwise, 0 is returned. */ static int nilfs_btree_root_broken(const struct nilfs_btree_node *node, - ino_t ino) + unsigned long ino) { int level, flags, nchildren; int ret = 0; -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] nilfs2: fix potential memory overrun on inode
On Fri, 20 Feb 2015 13:58:42 -0800, Andrew Morton wrote: > On Fri, 20 Feb 2015 22:46:35 +0900 Ryusuke Konishi > wrote: > >> Each inode of nilfs2 stores a root node of a b-tree, and it turned out >> to have a memory overrun issue: >> >> Each b-tree node of nilfs2 stores a set of key-value pairs and the >> number of them (in "bn_nchildren" member of nilfs_btree_node struct), >> as well as a few other "bn_*" members. >> >> Since the value of "bn_nchildren" is used for operations on the >> key-values within the b-tree node, it can cause memory access overrun >> if a large number is incorrectly set to "bn_nchildren". >> >> For instance, nilfs_btree_node_lookup() function determines the range >> of binary search with it, and too large "bn_nchildren" leads >> nilfs_btree_node_get_key() in that function to overrun. >> >> As for intermediate b-tree nodes, this is prevented by a sanity check >> performed when each node is read from a drive, however, no sanity >> check has been done for root nodes stored in inodes. >> >> This patch fixes the issue by adding missing sanity check against >> b-tree root nodes so that it's called when on-memory inodes are read >> from ifile, inode metadata file. > > How would one trigger this overrun? Mount an fs with a deliberately > corrupted/inconsistent fs image? Yes, this can be triggered by mounting an fs with a corrupted image deliberately or by chance. > Memory overrun sounds nasty so I'm thinking we add cc:stable to this > one. OK? Agreed. Ryusuke Konishi -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] nilfs2: fix potential memory overrun on inode
On Fri, 20 Feb 2015 22:46:35 +0900 Ryusuke Konishi wrote: > Each inode of nilfs2 stores a root node of a b-tree, and it turned out > to have a memory overrun issue: > > Each b-tree node of nilfs2 stores a set of key-value pairs and the > number of them (in "bn_nchildren" member of nilfs_btree_node struct), > as well as a few other "bn_*" members. > > Since the value of "bn_nchildren" is used for operations on the > key-values within the b-tree node, it can cause memory access overrun > if a large number is incorrectly set to "bn_nchildren". > > For instance, nilfs_btree_node_lookup() function determines the range > of binary search with it, and too large "bn_nchildren" leads > nilfs_btree_node_get_key() in that function to overrun. > > As for intermediate b-tree nodes, this is prevented by a sanity check > performed when each node is read from a drive, however, no sanity > check has been done for root nodes stored in inodes. > > This patch fixes the issue by adding missing sanity check against > b-tree root nodes so that it's called when on-memory inodes are read > from ifile, inode metadata file. How would one trigger this overrun? Mount an fs with a deliberately corrupted/inconsistent fs image? Memory overrun sounds nasty so I'm thinking we add cc:stable to this one. OK? -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] nilfs2: fix potential memory overrun on inode
Each inode of nilfs2 stores a root node of a b-tree, and it turned out to have a memory overrun issue: Each b-tree node of nilfs2 stores a set of key-value pairs and the number of them (in "bn_nchildren" member of nilfs_btree_node struct), as well as a few other "bn_*" members. Since the value of "bn_nchildren" is used for operations on the key-values within the b-tree node, it can cause memory access overrun if a large number is incorrectly set to "bn_nchildren". For instance, nilfs_btree_node_lookup() function determines the range of binary search with it, and too large "bn_nchildren" leads nilfs_btree_node_get_key() in that function to overrun. As for intermediate b-tree nodes, this is prevented by a sanity check performed when each node is read from a drive, however, no sanity check has been done for root nodes stored in inodes. This patch fixes the issue by adding missing sanity check against b-tree root nodes so that it's called when on-memory inodes are read from ifile, inode metadata file. Signed-off-by: Ryusuke Konishi --- fs/nilfs2/btree.c | 47 --- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c index b2e3ff3..c645d7c 100644 --- a/fs/nilfs2/btree.c +++ b/fs/nilfs2/btree.c @@ -31,6 +31,8 @@ #include "alloc.h" #include "dat.h" +static void __nilfs_btree_init(struct nilfs_bmap *bmap); + static struct nilfs_btree_path *nilfs_btree_alloc_path(void) { struct nilfs_btree_path *path; @@ -368,6 +370,34 @@ static int nilfs_btree_node_broken(const struct nilfs_btree_node *node, return ret; } +/** + * nilfs_btree_root_broken - verify consistency of btree root node + * @node: btree root node to be examined + * @ino: inode number + * + * Return Value: If node is broken, 1 is returned. Otherwise, 0 is returned. + */ +static int nilfs_btree_root_broken(const struct nilfs_btree_node *node, + ino_t ino) +{ + int level, flags, nchildren; + int ret = 0; + + level = nilfs_btree_node_get_level(node); + flags = nilfs_btree_node_get_flags(node); + nchildren = nilfs_btree_node_get_nchildren(node); + + if (unlikely(level < NILFS_BTREE_LEVEL_NODE_MIN || +level > NILFS_BTREE_LEVEL_MAX || +nchildren < 0 || +nchildren > NILFS_BTREE_ROOT_NCHILDREN_MAX)) { + pr_crit("NILFS: bad btree root (inode number=%lu): level = %d, flags = 0x%x, nchildren = %d\n", + ino, level, flags, nchildren); + ret = 1; + } + return ret; +} + int nilfs_btree_broken_node_block(struct buffer_head *bh) { int ret; @@ -1713,7 +1743,7 @@ nilfs_btree_commit_convert_and_insert(struct nilfs_bmap *btree, /* convert and insert */ dat = NILFS_BMAP_USE_VBN(btree) ? nilfs_bmap_get_dat(btree) : NULL; - nilfs_btree_init(btree); + __nilfs_btree_init(btree); if (nreq != NULL) { nilfs_bmap_commit_alloc_ptr(btree, dreq, dat); nilfs_bmap_commit_alloc_ptr(btree, nreq, dat); @@ -2294,12 +2324,23 @@ static const struct nilfs_bmap_operations nilfs_btree_ops_gc = { .bop_gather_data= NULL, }; -int nilfs_btree_init(struct nilfs_bmap *bmap) +static void __nilfs_btree_init(struct nilfs_bmap *bmap) { bmap->b_ops = &nilfs_btree_ops; bmap->b_nchildren_per_block = NILFS_BTREE_NODE_NCHILDREN_MAX(nilfs_btree_node_size(bmap)); - return 0; +} + +int nilfs_btree_init(struct nilfs_bmap *bmap) +{ + int ret = 0; + + __nilfs_btree_init(bmap); + + if (nilfs_btree_root_broken(nilfs_btree_get_root(bmap), + bmap->b_inode->i_ino)) + ret = -EIO; + return ret; } void nilfs_btree_init_gc(struct nilfs_bmap *bmap) -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/1] nilfs2: fix potential memory overrun on inode
Hi Andrew, please queue the following patch as a bug fix. It fixes a memory overrun issue recently I found in the b-tree implementation of nilfs2. Thanks, Ryusuke Konishi -- Ryusuke Konishi (1): nilfs2: fix potential memory overrun on inode fs/nilfs2/btree.c | 47 --- 1 file changed, 44 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html