Messages in this thread | | | From | Colin Ian King <> | Subject | re: f2fs: add F2FS_IOC_DECOMPRESS_FILE and F2FS_IOC_COMPRESS_FILE | Date | Mon, 4 Jan 2021 14:24:39 +0000 |
| |
Hi,
Static analysis using Coverity has detected a potential null pointer dereference after a null check in the following commit:
commit 5fdb322ff2c2b4ad519f490dcb7ebb96c5439af7 Author: Daeho Jeong <daehojeong@google.com> Date: Thu Dec 3 15:56:15 2020 +0900
f2fs: add F2FS_IOC_DECOMPRESS_FILE and F2FS_IOC_COMPRESS_FILE
The analysis is as follows:
4025 static int redirty_blocks(struct inode *inode, pgoff_t page_idx, int len) 4026 { 4027 DEFINE_READAHEAD(ractl, NULL, inode->i_mapping, page_idx); 4028 struct address_space *mapping = inode->i_mapping; 4029 struct page *page; 4030 pgoff_t redirty_idx = page_idx; 4031 int i, page_len = 0, ret = 0; 4032 4033 page_cache_ra_unbounded(&ractl, len, 0); 4034
1. Condition i < len, taking true branch. 4. Condition i < len, taking true branch.
4035 for (i = 0; i < len; i++, page_idx++) { 4036 page = read_cache_page(mapping, page_idx, NULL, NULL);
2. Condition IS_ERR(page), taking false branch. 5. Condition IS_ERR(page), taking true branch.
4037 if (IS_ERR(page)) { 4038 ret = PTR_ERR(page);
6. Breaking from loop.
4039 break; 4040 } 4041 page_len++;
3. Jumping back to the beginning of the loop.
4042 } 4043
7. Condition i < page_len, taking true branch.
4044 for (i = 0; i < page_len; i++, redirty_idx++) { 4045 page = find_lock_page(mapping, redirty_idx);
8. Condition !page, taking true branch. 9. var_compare_op: Comparing page to null implies that page might be null.
4046 if (!page) 4047 ret = -ENOENT;
Dereference after null check (FORWARD_NULL)
10. var_deref_model: Passing null pointer page to set_page_dirty, which dereferences it.
4048 set_page_dirty(page); 4049 f2fs_put_page(page, 1); 4050 f2fs_put_page(page, 0); 4051 } 4052 4053 return ret; 4054 }
The !page check on line 4046 sets ret appropriately but we have a following call to set_page_dirty on a null page that causes the error. Not sure how this should be fixed, should the check bail out immediately or just avoid the following set_page_dirty anf f2fs_put_page calls?
Colin
| |