| Message ID | 20240526110631.10618-1-llfamsec@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: add bounds checking to ocfs2_search_dirblock() | expand |
On 5/26/24 7:06 PM, lei lu wrote: > Add a check to make sure all members of the ocfs2_dir_entry > don't stray beyond valid memory region. > > Signed-off-by: lei lu <llfamsec@gmail.com> > --- > fs/ocfs2/dir.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index d620d4c53c6f..385576d86983 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -358,6 +358,17 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, > > de = (struct ocfs2_dir_entry *) de_buf; > > + if (unlikely(de_buf + OCFS2_DIR_MEMBER_LEN > dlimit)) { > + ret = -1; > + goto bail; > + } It seems that this check can be moved to: while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { ... } > + de_len = le16_to_cpu(de->rec_len); > + if (unlikely(de_buf + de_len > dlimit) || dlimit = de_buf + bytes; So here you mean check de_len < bytes? Or should we check more precisely, like in ocfs2_check_dir_entry(): (de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len) > + unlikely(de_len < OCFS2_DIR_REC_LEN(de->name_len))) { This is already done in ocfs2_check_dir_entry(). > + ret = -1; > + goto bail; > + } > + > if (de_buf + namelen <= dlimit && > ocfs2_match(namelen, name, de)) { > /* found a match - just to be sure, do a full check */ > @@ -371,7 +382,6 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, > } > > /* prevent looping on a bad block */ > - de_len = le16_to_cpu(de->rec_len); > if (de_len <= 0) { > ret = -1; > goto bail;
On 5/27/24 11:19 AM, lei lu wrote: > code in ocfs2_search_dirblock: > if (de_buf + namelen <= dlimit && > ocfs2_match(namelen, name, de)) { > > I'm not sure what "de_buf + namelen < dlimit" means. I think "de_buf + > namelen" maybe "<= dlimit", but "((struct ocfs2_dir_entry *) de_buf)->name > + namelen" maybe "> dlimit". Or maybe we should check "de->name + namelen > <= dlimit" here? > Yes, I think you are right. de->name + namelen is a more precisely check. Thanks, Joseph > lei lu <llfamsec@gmail.com> 于2024年5月27日周一 11:11写道： > >>> It seems that this check can be moved to: >>> while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { >>> ... >>> } >> >> Right, it also work. >> >>> dlimit = de_buf + bytes; >>> So here you mean check de_len < bytes? >>> >>> Or should we check more precisely, like in ocfs2_check_dir_entry(): >>> (de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len) >> >> "de_buf + OCFS2_DIR_MEMBER_LEN > dlimit" checks the fixed members don't >> stray beyond valid memory region, and "de_buf + de_len > dlimit" for the >> whole de. You should mean "((char *) de - bh->b_data) + de_len > dlimit" >> which is similar to "((char *) de - bh->b_data) + rlen > >> dir->i_sb->s_blocksize)" in ocfs2_check_dir_entry. But dlimit which is end >> of the de list may not reach the block end. >> >>> This is already done in ocfs2_check_dir_entry(). >> >> Right, We need to check "de_len < OCFS2_DIR_REC_LEN(de->name_len)" before >> "ocfs2_match(namelen, name, de)", because if de->name_len is too large, I >> think maybe trigger anther OOB in ocfs2_match. Or we should call >> ocfs2_check_dir_entry earlier to do a full check for de. >> >> Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月27日周一 10:33写道： >> >>> >>> >>> On 5/26/24 7:06 PM, lei lu wrote: >>>> Add a check to make sure all members of the ocfs2_dir_entry >>>> don't stray beyond valid memory region. >>>> >>>> Signed-off-by: lei lu <llfamsec@gmail.com> >>>> --- >>>> fs/ocfs2/dir.c | 12 +++++++++++- >>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >>>> index d620d4c53c6f..385576d86983 100644 >>>> --- a/fs/ocfs2/dir.c >>>> +++ b/fs/ocfs2/dir.c >>>> @@ -358,6 +358,17 @@ static inline int ocfs2_search_dirblock(struct >>> buffer_head *bh, >>>> >>>> de = (struct ocfs2_dir_entry *) de_buf; >>>> >>>> + if (unlikely(de_buf + OCFS2_DIR_MEMBER_LEN > dlimit)) { >>>> + ret = -1; >>>> + goto bail; >>>> + } >>> >>> It seems that this check can be moved to: >>> while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { >>> ... >>> } >>> >>>> + de_len = le16_to_cpu(de->rec_len); >>>> + if (unlikely(de_buf + de_len > dlimit) || >>> >>> dlimit = de_buf + bytes; >>> So here you mean check de_len < bytes? >>> >>> Or should we check more precisely, like in ocfs2_check_dir_entry(): >>> (de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len) >>> >>>> + unlikely(de_len < OCFS2_DIR_REC_LEN(de->name_len))) { >>> >>> This is already done in ocfs2_check_dir_entry(). >>> >>>> + ret = -1; >>>> + goto bail; >>>> + } >>>> + >>>> if (de_buf + namelen <= dlimit && >>>> ocfs2_match(namelen, name, de)) { >>>> /* found a match - just to be sure, do a full >>> check */ >>>> @@ -371,7 +382,6 @@ static inline int ocfs2_search_dirblock(struct >>> buffer_head *bh, >>>> } >>>> >>>> /* prevent looping on a bad block */ >>>> - de_len = le16_to_cpu(de->rec_len); >>>> if (de_len <= 0) { >>>> ret = -1; >>>> goto bail; >>> >> >
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index d620d4c53c6f..385576d86983 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -358,6 +358,17 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, de = (struct ocfs2_dir_entry *) de_buf; + if (unlikely(de_buf + OCFS2_DIR_MEMBER_LEN > dlimit)) { + ret = -1; + goto bail; + } + de_len = le16_to_cpu(de->rec_len); + if (unlikely(de_buf + de_len > dlimit) || + unlikely(de_len < OCFS2_DIR_REC_LEN(de->name_len))) { + ret = -1; + goto bail; + } + if (de_buf + namelen <= dlimit && ocfs2_match(namelen, name, de)) { /* found a match - just to be sure, do a full check */ @@ -371,7 +382,6 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, } /* prevent looping on a bad block */ - de_len = le16_to_cpu(de->rec_len); if (de_len <= 0) { ret = -1; goto bail;
Add a check to make sure all members of the ocfs2_dir_entry don't stray beyond valid memory region. Signed-off-by: lei lu <llfamsec@gmail.com> --- fs/ocfs2/dir.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)