| Message ID | tencent_EC9ACDC0793A6F742D7D6FA094F0E96AEF0A@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: Add i_size check for dir | expand |
On 8/20/24 8:08 PM, Edward Adam Davis wrote: > When the i_size of dir is too large, it will cause limit to overflow and > be less than de_buf, ultimately resulting in last_de not being initialized > and causing uaf issue. > > Reported-and-tested-by: syzbot+5a64828fcc4c2ad9b04f@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > fs/ocfs2/dir.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index d620d4c53c6f..c308dba6d213 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -3343,6 +3343,8 @@ static int ocfs2_find_dir_space_id(struct inode *dir, struct buffer_head *di_bh, > unsigned long offset = 0; > unsigned int rec_len, new_rec_len, free_space; > > + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) > + return -EINVAL; Why OCFS2_MAX_BLOCKSIZE? It seems that this is caused by a corrupted dir inode, since this is an inline case, we may try best to make sure it won't exceeds block size? i.e. dir->i_sb->s_blocksize. Thanks, Joseph > /* > * This calculates how many free bytes we'd have in block zero, should > * this function force expansion to an extent tree.
On Tue, 20 Aug 2024 20:44:37 +0800, Joseph Qi wrote: > > When the i_size of dir is too large, it will cause limit to overflow and > > be less than de_buf, ultimately resulting in last_de not being initialized > > and causing uaf issue. > > > > Reported-and-tested-by: syzbot+5a64828fcc4c2ad9b04f@syzkaller.appspotmail.com > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > fs/ocfs2/dir.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > > index d620d4c53c6f..c308dba6d213 100644 > > --- a/fs/ocfs2/dir.c > > +++ b/fs/ocfs2/dir.c > > @@ -3343,6 +3343,8 @@ static int ocfs2_find_dir_space_id(struct inode *dir, struct buffer_head *di_bh, > > unsigned long offset = 0; > > unsigned int rec_len, new_rec_len, free_space; > > > > + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) > > + return -EINVAL; > > Why OCFS2_MAX_BLOCKSIZE? I think it is largest block size in ocfs2, therefore, if it is larger than it, it must be incorrect, even though the value of i_size in dir in the current issue is much larger than it (i_size_read(dir) is 0x900000000000100). > It seems that this is caused by a corrupted dir inode, since this is an > inline case, we may try best to make sure it won't exceeds block size? > i.e. dir->i_sb->s_blocksize. You mean dir->i_sb->s_blocksize bigger than OCFS2_MAX_BLOCKSIZE? BR, Edward
On Tue, Aug 20, 2024 at 08:08:38PM +0800, Edward Adam Davis wrote: > When the i_size of dir is too large, it will cause limit to overflow and > be less than de_buf, ultimately resulting in last_de not being initialized > and causing uaf issue. > > + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) > + return -EINVAL; Surely directories can be more than one block in size?
On 8/20/24 22:59, Matthew Wilcox wrote: > On Tue, Aug 20, 2024 at 08:08:38PM +0800, Edward Adam Davis wrote: >> When the i_size of dir is too large, it will cause limit to overflow and >> be less than de_buf, ultimately resulting in last_de not being initialized >> and causing uaf issue. >> >> + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) >> + return -EINVAL; > > Surely directories can be more than one block in size? > The key point above is that the patch uses a hard code value, but in the real world, the blocksize can be smaller than OCFS2_MAX_BLOCKSIZE. -Heming
On Tue, 20 Aug 2024 15:59:56 +0100, Matthew Wilcox wrote: > > When the i_size of dir is too large, it will cause limit to overflow and > > be less than de_buf, ultimately resulting in last_de not being initialized > > and causing uaf issue. > > > > + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) > > + return -EINVAL; > > Surely directories can be more than one block in size? In this issue, Yes. -- Edward
On 8/20/24 9:55 PM, Edward Adam Davis wrote: > On Tue, 20 Aug 2024 20:44:37 +0800, Joseph Qi wrote: >>> When the i_size of dir is too large, it will cause limit to overflow and >>> be less than de_buf, ultimately resulting in last_de not being initialized >>> and causing uaf issue. >>> >>> Reported-and-tested-by: syzbot+5a64828fcc4c2ad9b04f@syzkaller.appspotmail.com >>> Signed-off-by: Edward Adam Davis <eadavis@qq.com> >>> --- >>> fs/ocfs2/dir.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >>> index d620d4c53c6f..c308dba6d213 100644 >>> --- a/fs/ocfs2/dir.c >>> +++ b/fs/ocfs2/dir.c >>> @@ -3343,6 +3343,8 @@ static int ocfs2_find_dir_space_id(struct inode *dir, struct buffer_head *di_bh, >>> unsigned long offset = 0; >>> unsigned int rec_len, new_rec_len, free_space; >>> >>> + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) >>> + return -EINVAL; >> >> Why OCFS2_MAX_BLOCKSIZE? > I think it is largest block size in ocfs2, therefore, if it is larger > than it, it must be incorrect, even though the value of i_size in dir > in the current issue is much larger than it (i_size_read(dir) is 0x900000000000100). >> It seems that this is caused by a corrupted dir inode, since this is an >> inline case, we may try best to make sure it won't exceeds block size? >> i.e. dir->i_sb->s_blocksize. > You mean dir->i_sb->s_blocksize bigger than OCFS2_MAX_BLOCKSIZE? > No, I mean check s_blocksize seems more reasonable rather than OCFS2_MAX_BLOCKSIZE. Thanks, Joseph
On Wed, 21 Aug 2024 18:41:06 +0800, Joseph Qi wrote: >>>> When the i_size of dir is too large, it will cause limit to overflow and >>>> be less than de_buf, ultimately resulting in last_de not being initialized >>>> and causing uaf issue. >>>> >>>> Reported-and-tested-by: syzbot+5a64828fcc4c2ad9b04f@syzkaller.appspotmail.com >>>> Signed-off-by: Edward Adam Davis <eadavis@qq.com> >>>> --- >>>> fs/ocfs2/dir.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >>>> index d620d4c53c6f..c308dba6d213 100644 >>>> --- a/fs/ocfs2/dir.c >>>> +++ b/fs/ocfs2/dir.c >>>> @@ -3343,6 +3343,8 @@ static int ocfs2_find_dir_space_id(struct inode *dir, struct buffer_head *di_bh, >>>> unsigned long offset = 0; >>>> unsigned int rec_len, new_rec_len, free_space; >>>> >>>> + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) >>>> + return -EINVAL; >>> >>> Why OCFS2_MAX_BLOCKSIZE? >> I think it is largest block size in ocfs2, therefore, if it is larger >> than it, it must be incorrect, even though the value of i_size in dir >> in the current issue is much larger than it (i_size_read(dir) is 0x900000000000100). >>> It seems that this is caused by a corrupted dir inode, since this is an >>> inline case, we may try best to make sure it won't exceeds block size? >>> i.e. dir->i_sb->s_blocksize. >> You mean dir->i_sb->s_blocksize bigger than OCFS2_MAX_BLOCKSIZE? >> >No, I mean check s_blocksize seems more reasonable rather than >OCFS2_MAX_BLOCKSIZE. Perhaps we have different perspectives on the issue. My approach is to set a bottom line for the dir's i_size, and if it exceeds the bottom line, the dir will definitely be corrupted. And I think OCFS2_MAX_BLOCKSIZE is the reasonable bottom line. BR, Edward
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index d620d4c53c6f..c308dba6d213 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -3343,6 +3343,8 @@ static int ocfs2_find_dir_space_id(struct inode *dir, struct buffer_head *di_bh, unsigned long offset = 0; unsigned int rec_len, new_rec_len, free_space; + if (i_size_read(dir) > OCFS2_MAX_BLOCKSIZE) + return -EINVAL; /* * This calculates how many free bytes we'd have in block zero, should * this function force expansion to an extent tree.
When the i_size of dir is too large, it will cause limit to overflow and be less than de_buf, ultimately resulting in last_de not being initialized and causing uaf issue. Reported-and-tested-by: syzbot+5a64828fcc4c2ad9b04f@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/ocfs2/dir.c | 2 ++ 1 file changed, 2 insertions(+)