| Message ID | 20180719163548.22448-2-colin.king@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | f2fs: fix a dead loop in f2fs_fiemap() Content-Type: | expand |
On 07/19/2018 06:35 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> From: Wei Fang <fangwei1@huawei.com> > CVE-2017-18257 > > A dead loop can be triggered in f2fs_fiemap() using the test case > as below: > > ... > fd = open(); > fallocate(fd, 0, 0, 4294967296); > ioctl(fd, FS_IOC_FIEMAP, fiemap_buf); > ... > > It's caused by an overflow in __get_data_block(): > ... > bh->b_size = map.m_len << inode->i_blkbits; > ... > map.m_len is an unsigned int, and bh->b_size is a size_t which is 64 bits > on 64 bits archtecture, type conversion from an unsigned int to a size_t > will result in an overflow. > > In the above-mentioned case, bh->b_size will be zero, and f2fs_fiemap() > will call get_data_block() at block 0 again an again. > > Fix this by adding a force conversion before left shift. > > Signed-off-by: Wei Fang <fangwei1@huawei.com> > Acked-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > (backported from upstream commit b86e33075ed1909d8002745b56ecf73b833db143) (backported from commit b86e33075ed1909d8002745b56ecf73b833db143) [cking: <what were the changes from the original commit?>] > Signed-off-by: Colin Ian King <colin.king@canonical.com> Acked-by: Juerg Haefliger <juergh@canonical.com> ...Juerg > --- > fs/f2fs/data.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index aa3438c..884bccc 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode *inode, sector_t iblock, > != (dn.data_blkaddr + i)) || maxblocks == i) > break; > map_bh(bh_result, inode->i_sb, dn.data_blkaddr); > - bh_result->b_size = (i << blkbits); > + bh_result->b_size = ((u64)i << blkbits); > } > f2fs_put_dnode(&dn); > trace_f2fs_get_data_block(inode, iblock, bh_result, 0); >
On 19.07.2018 18:35, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > CVE-2017-18257 > > A dead loop can be triggered in f2fs_fiemap() using the test case > as below: > > ... > fd = open(); > fallocate(fd, 0, 0, 4294967296); > ioctl(fd, FS_IOC_FIEMAP, fiemap_buf); > ... > > It's caused by an overflow in __get_data_block(): > ... > bh->b_size = map.m_len << inode->i_blkbits; > ... > map.m_len is an unsigned int, and bh->b_size is a size_t which is 64 bits > on 64 bits archtecture, type conversion from an unsigned int to a size_t > will result in an overflow. > > In the above-mentioned case, bh->b_size will be zero, and f2fs_fiemap() > will call get_data_block() at block 0 again an again. > > Fix this by adding a force conversion before left shift. > > Signed-off-by: Wei Fang <fangwei1@huawei.com> > Acked-by: Chao Yu <yuchao0@huawei.com> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > (backported from upstream commit b86e33075ed1909d8002745b56ecf73b833db143) > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- I think that Trusty actually is just not affected and the tracker needs to have hinting adjusted. fiemap looks to be only supported since v3.16: Author: Jaegeuk Kim <jaegeuk@kernel.org> Date: Sun Jun 8 04:30:14 2014 +0900 f2fs: support f2fs_fiemap The replaced line actually has both blkbits and i as ints, so no conversion happens there. -Stefan > fs/f2fs/data.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index aa3438c..884bccc 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode *inode, sector_t iblock, > != (dn.data_blkaddr + i)) || maxblocks == i) > break; > map_bh(bh_result, inode->i_sb, dn.data_blkaddr); > - bh_result->b_size = (i << blkbits); > + bh_result->b_size = ((u64)i << blkbits); > } > f2fs_put_dnode(&dn); > trace_f2fs_get_data_block(inode, iblock, bh_result, 0); >
On Wed, 25 Jul 2018 17:39:27 +0200 Stefan Bader <stefan.bader@canonical.com> wrote: > On 19.07.2018 18:35, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > CVE-2017-18257 > > > > A dead loop can be triggered in f2fs_fiemap() using the test case > > as below: > > > > ... > > fd = open(); > > fallocate(fd, 0, 0, 4294967296); > > ioctl(fd, FS_IOC_FIEMAP, fiemap_buf); > > ... > > > > It's caused by an overflow in __get_data_block(): > > ... > > bh->b_size = map.m_len << inode->i_blkbits; > > ... > > map.m_len is an unsigned int, and bh->b_size is a size_t which is > > 64 bits on 64 bits archtecture, type conversion from an unsigned > > int to a size_t will result in an overflow. > > > > In the above-mentioned case, bh->b_size will be zero, and > > f2fs_fiemap() will call get_data_block() at block 0 again an again. > > > > Fix this by adding a force conversion before left shift. > > > > Signed-off-by: Wei Fang <fangwei1@huawei.com> > > Acked-by: Chao Yu <yuchao0@huawei.com> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > (backported from upstream commit > > b86e33075ed1909d8002745b56ecf73b833db143) Signed-off-by: Colin Ian > > King <colin.king@canonical.com> --- > > I think that Trusty actually is just not affected and the tracker > needs to have hinting adjusted. fiemap looks to be only supported > since v3.16: OK. > Author: Jaegeuk Kim <jaegeuk@kernel.org> > Date: Sun Jun 8 04:30:14 2014 +0900 > > f2fs: support f2fs_fiemap > > The replaced line actually has both blkbits and i as ints, so no > conversion happens there. But if the left-shifted value is bigger than 32bit we get an overflow without casting. Or am I missing something? ...Juerg > -Stefan > > > fs/f2fs/data.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index aa3438c..884bccc 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode > > *inode, sector_t iblock, != (dn.data_blkaddr + i)) || maxblocks == > > i) break; > > map_bh(bh_result, inode->i_sb, dn.data_blkaddr); > > - bh_result->b_size = (i << blkbits); > > + bh_result->b_size = ((u64)i << blkbits); > > } > > f2fs_put_dnode(&dn); > > trace_f2fs_get_data_block(inode, iblock, bh_result, 0); > > > >
On 26.07.2018 13:41, Juerg Haefliger wrote: > On Wed, 25 Jul 2018 17:39:27 +0200 > Stefan Bader <stefan.bader@canonical.com> wrote: > >> On 19.07.2018 18:35, Colin King wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> CVE-2017-18257 >>> >>> A dead loop can be triggered in f2fs_fiemap() using the test case >>> as below: >>> >>> ... >>> fd = open(); >>> fallocate(fd, 0, 0, 4294967296); >>> ioctl(fd, FS_IOC_FIEMAP, fiemap_buf); >>> ... >>> >>> It's caused by an overflow in __get_data_block(): >>> ... >>> bh->b_size = map.m_len << inode->i_blkbits; >>> ... >>> map.m_len is an unsigned int, and bh->b_size is a size_t which is >>> 64 bits on 64 bits archtecture, type conversion from an unsigned >>> int to a size_t will result in an overflow. >>> >>> In the above-mentioned case, bh->b_size will be zero, and >>> f2fs_fiemap() will call get_data_block() at block 0 again an again. >>> >>> Fix this by adding a force conversion before left shift. >>> >>> Signed-off-by: Wei Fang <fangwei1@huawei.com> >>> Acked-by: Chao Yu <yuchao0@huawei.com> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> (backported from upstream commit >>> b86e33075ed1909d8002745b56ecf73b833db143) Signed-off-by: Colin Ian >>> King <colin.king@canonical.com> --- >> >> I think that Trusty actually is just not affected and the tracker >> needs to have hinting adjusted. fiemap looks to be only supported >> since v3.16: > > OK. > > >> Author: Jaegeuk Kim <jaegeuk@kernel.org> >> Date: Sun Jun 8 04:30:14 2014 +0900 >> >> f2fs: support f2fs_fiemap >> >> The replaced line actually has both blkbits and i as ints, so no >> conversion happens there. > > But if the left-shifted value is bigger than 32bit we get an overflow > without casting. Or am I missing something? Hm, no, you are right. I was mis-reading which part was 64bit. Still hard to say whether it is possible to call get_data_block_ro() in any way that lets i and blkbits have values that overflow. And then the result has to be used to make incremental progress ... I cannot say it never happens but is that then covered by this cve still? -Stefan > > ...Juerg > > >> -Stefan >> >>> fs/f2fs/data.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index aa3438c..884bccc 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode >>> *inode, sector_t iblock, != (dn.data_blkaddr + i)) || maxblocks == >>> i) break; >>> map_bh(bh_result, inode->i_sb, dn.data_blkaddr); >>> - bh_result->b_size = (i << blkbits); >>> + bh_result->b_size = ((u64)i << blkbits); >>> } >>> f2fs_put_dnode(&dn); >>> trace_f2fs_get_data_block(inode, iblock, bh_result, 0); >>> >> >> >
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index aa3438c..884bccc 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -455,7 +455,7 @@ static int get_data_block_ro(struct inode *inode, sector_t iblock, != (dn.data_blkaddr + i)) || maxblocks == i) break; map_bh(bh_result, inode->i_sb, dn.data_blkaddr); - bh_result->b_size = (i << blkbits); + bh_result->b_size = ((u64)i << blkbits); } f2fs_put_dnode(&dn); trace_f2fs_get_data_block(inode, iblock, bh_result, 0);