| Message ID | 20190724021132.27378-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: btrfs: Fix a possible null-pointer dereference in insert_inline_extent() | expand |
On 2019/7/24 上午10:11, Jia-Ju Bai wrote: > In insert_inline_extent(), there is an if statement on line 181 to check > whether compressed_pages is NULL: > if (compressed_size && compressed_pages) > > When compressed_pages is NULL, compressed_pages is used on line 215: > cpage = compressed_pages[i]; > > Thus, a possible null-pointer dereference may occur. > > To fix this possible bug, compressed_pages is checked on line 214. This can only be hit with compressed_size > 0 and compressed_pages != NULL. It would be better to have an extra ASSERT() to warn developers about the impossible case. Thanks, Qu > > This bug is found by a static analysis tool STCheck written by us. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 1af069a9a0c7..19182272fbd8 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -211,7 +211,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans, > if (compress_type != BTRFS_COMPRESS_NONE) { > struct page *cpage; > int i = 0; > - while (compressed_size > 0) { > + while (compressed_size > 0 && compressed_pages) { > cpage = compressed_pages[i]; > cur_size = min_t(unsigned long, compressed_size, > PAGE_SIZE); >
On 2019/7/24 10:21, Qu Wenruo wrote: > > On 2019/7/24 上午10:11, Jia-Ju Bai wrote: >> In insert_inline_extent(), there is an if statement on line 181 to check >> whether compressed_pages is NULL: >> if (compressed_size && compressed_pages) >> >> When compressed_pages is NULL, compressed_pages is used on line 215: >> cpage = compressed_pages[i]; >> >> Thus, a possible null-pointer dereference may occur. >> >> To fix this possible bug, compressed_pages is checked on line 214. > This can only be hit with compressed_size > 0 and compressed_pages != NULL. > > It would be better to have an extra ASSERT() to warn developers about > the impossible case. Thanks for the reply :) So I should add ASSERT(compressed_size > 0 & compressed_pages) at the beginning of the function, and remove "if (compressed_size && compressed_pages)"? Best wishes, Jia-Ju Bai
On 2019/7/24 上午10:33, Jia-Ju Bai wrote: > > > On 2019/7/24 10:21, Qu Wenruo wrote: >> >> On 2019/7/24 上午10:11, Jia-Ju Bai wrote: >>> In insert_inline_extent(), there is an if statement on line 181 to check >>> whether compressed_pages is NULL: >>> if (compressed_size && compressed_pages) >>> >>> When compressed_pages is NULL, compressed_pages is used on line 215: >>> cpage = compressed_pages[i]; >>> >>> Thus, a possible null-pointer dereference may occur. >>> >>> To fix this possible bug, compressed_pages is checked on line 214. >> This can only be hit with compressed_size > 0 and compressed_pages != >> NULL. >> >> It would be better to have an extra ASSERT() to warn developers about >> the impossible case. > > Thanks for the reply :) > So I should add ASSERT(compressed_size > 0 & compressed_pages) at the > beginning of the function, and remove "if (compressed_size && > compressed_pages)"? My suggestion is, ASSERT((compressed_size >0 && compressed_pages) || (compressed_size == 0 && !compressed_pages)) And keeps the original checks. Anyway, just a suggestion. Thanks, Qu > > > Best wishes, > Jia-Ju Bai
On Wed, Jul 24, 2019 at 10:57:24AM +0800, Qu Wenruo wrote: > On 2019/7/24 上午10:33, Jia-Ju Bai wrote: > > On 2019/7/24 10:21, Qu Wenruo wrote: > >> On 2019/7/24 上午10:11, Jia-Ju Bai wrote: > >>> In insert_inline_extent(), there is an if statement on line 181 to check > >>> whether compressed_pages is NULL: > >>> if (compressed_size && compressed_pages) > >>> > >>> When compressed_pages is NULL, compressed_pages is used on line 215: > >>> cpage = compressed_pages[i]; > >>> > >>> Thus, a possible null-pointer dereference may occur. > >>> > >>> To fix this possible bug, compressed_pages is checked on line 214. > >> This can only be hit with compressed_size > 0 and compressed_pages != > >> NULL. > >> > >> It would be better to have an extra ASSERT() to warn developers about > >> the impossible case. > > > > Thanks for the reply :) > > So I should add ASSERT(compressed_size > 0 & compressed_pages) at the > > beginning of the function, and remove "if (compressed_size && > > compressed_pages)"? > > My suggestion is, ASSERT((compressed_size >0 && compressed_pages) || > (compressed_size == 0 && !compressed_pages)) > > And keeps the original checks. > > Anyway, just a suggestion. Agreed, the assertion would be good, covering both cases in one statement at the beginning of the function.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 1af069a9a0c7..19182272fbd8 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -211,7 +211,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans, if (compress_type != BTRFS_COMPRESS_NONE) { struct page *cpage; int i = 0; - while (compressed_size > 0) { + while (compressed_size > 0 && compressed_pages) { cpage = compressed_pages[i]; cur_size = min_t(unsigned long, compressed_size, PAGE_SIZE);
In insert_inline_extent(), there is an if statement on line 181 to check whether compressed_pages is NULL: if (compressed_size && compressed_pages) When compressed_pages is NULL, compressed_pages is used on line 215: cpage = compressed_pages[i]; Thus, a possible null-pointer dereference may occur. To fix this possible bug, compressed_pages is checked on line 214. This bug is found by a static analysis tool STCheck written by us. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)