* [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock() @ 2024-05-26 11:06 lei lu 2024-05-27 2:33 ` Joseph Qi 0 siblings, 1 reply; 3+ messages in thread From: lei lu @ 2024-05-26 11:06 UTC (permalink / raw) To: joseph.qi; +Cc: mark, jlbec, ocfs2-devel, lei lu 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; + } + 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; -- 2.34.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock() 2024-05-26 11:06 [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock() lei lu @ 2024-05-27 2:33 ` Joseph Qi [not found] ` <CAEBF3_b6uSbo5s6dk=Vvt+mYzSjzzx0RMnaHQcc+YdWco6Dpzw@mail.gmail.com> 0 siblings, 1 reply; 3+ messages in thread From: Joseph Qi @ 2024-05-27 2:33 UTC (permalink / raw) To: lei lu; +Cc: mark, jlbec, ocfs2-devel, Greg Kroah-Hartman 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; ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <CAEBF3_b6uSbo5s6dk=Vvt+mYzSjzzx0RMnaHQcc+YdWco6Dpzw@mail.gmail.com>]
[parent not found: <CAEBF3_anUzNk6gwLG0d=_5QyWeStvkmp_nT9Z_k9fFdjvJ-wiQ@mail.gmail.com>]
* Re: [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock() [not found] ` <CAEBF3_anUzNk6gwLG0d=_5QyWeStvkmp_nT9Z_k9fFdjvJ-wiQ@mail.gmail.com> @ 2024-05-27 3:32 ` Joseph Qi 0 siblings, 0 replies; 3+ messages in thread From: Joseph Qi @ 2024-05-27 3:32 UTC (permalink / raw) To: lei lu; +Cc: mark, jlbec, ocfs2-devel, Greg Kroah-Hartman 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; >>> >> > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-27 3:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-26 11:06 [PATCH] ocfs2: add bounds checking to ocfs2_search_dirblock() lei lu
2024-05-27 2:33 ` Joseph Qi
[not found] ` <CAEBF3_b6uSbo5s6dk=Vvt+mYzSjzzx0RMnaHQcc+YdWco6Dpzw@mail.gmail.com>
[not found] ` <CAEBF3_anUzNk6gwLG0d=_5QyWeStvkmp_nT9Z_k9fFdjvJ-wiQ@mail.gmail.com>
2024-05-27 3:32 ` Joseph Qi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.