| Message ID | 20240528120140.15550-1-llfamsec@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ocfs2: add bounds checking to ocfs2_search_dirblock() | expand |
On 5/28/24 8:01 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 | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index d620d4c53c6f..56e2f8c62106 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -352,19 +352,20 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, > de_buf = first_de; > dlimit = de_buf + bytes; > > - while (de_buf < dlimit) { > + while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { > /* this code is executed quadratically often */ > /* do minimal checking `by hand' */ > > de = (struct ocfs2_dir_entry *) de_buf; > > - if (de_buf + namelen <= dlimit && > + /* do a full check */ > + if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > + ret = -1; > + goto bail; > + } What's the difference by moving full check above? As I commented in v1, we may add a more check in ocfs2_check_dir_entry(). > + > + if (de->name + namelen <= dlimit && As discussed in v1, this change looks reasonable. Thanks, Joseph > ocfs2_match(namelen, name, de)) { > - /* found a match - just to be sure, do a full check */ > - if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > - ret = -1; > - goto bail; > - } > *res_dir = de; > ret = 1; > goto bail;
Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月29日周三 09:33写道： > > > > On 5/28/24 8:01 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 | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > > index d620d4c53c6f..56e2f8c62106 100644 > > --- a/fs/ocfs2/dir.c > > +++ b/fs/ocfs2/dir.c > > @@ -352,19 +352,20 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, > > de_buf = first_de; > > dlimit = de_buf + bytes; > > > > - while (de_buf < dlimit) { > > + while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { > > /* this code is executed quadratically often */ > > /* do minimal checking `by hand' */ > > > > de = (struct ocfs2_dir_entry *) de_buf; > > > > - if (de_buf + namelen <= dlimit && > > + /* do a full check */ > > + if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > > + ret = -1; > > + goto bail; > > + } > > What's the difference by moving full check above? > As I commented in v1, we may add a more check in ocfs2_check_dir_entry(). Thanks for your time. If no match is found, there is no check for de. I'm not sure if there will be any OOB elsewhere when "de->rec_len < OCFS2_DIR_REC_LEN(de->name_len)". I'm not sure what "(de - bh->b_data) + de_len > bytes - OCFS2_DIR_REC_LEN(de->name_len)" means in V1, bh->b_data is the start offset of ocfs2_dinode. bytes is just the size of the de list. Maybe we should check "((char *) de - bh->b_data) + rlen > sizeof(struct ocfs2_dinode) + bytes" which is same as "((char *) de - bh->b_data) + rlen > dlimit". > > > + > > + if (de->name + namelen <= dlimit && > > As discussed in v1, this change looks reasonable. > > Thanks, > Joseph > > > ocfs2_match(namelen, name, de)) { > > - /* found a match - just to be sure, do a full check */ > > - if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > > - ret = -1; > > - goto bail; > > - } > > *res_dir = de; > > ret = 1; > > goto bail;
On 5/29/24 11:18 AM, lei lu wrote: > Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月29日周三 09:33写道： >> >> >> >> On 5/28/24 8:01 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 | 15 ++++++++------- >>> 1 file changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >>> index d620d4c53c6f..56e2f8c62106 100644 >>> --- a/fs/ocfs2/dir.c >>> +++ b/fs/ocfs2/dir.c >>> @@ -352,19 +352,20 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, >>> de_buf = first_de; >>> dlimit = de_buf + bytes; >>> >>> - while (de_buf < dlimit) { >>> + while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { >>> /* this code is executed quadratically often */ >>> /* do minimal checking `by hand' */ >>> >>> de = (struct ocfs2_dir_entry *) de_buf; >>> >>> - if (de_buf + namelen <= dlimit && >>> + /* do a full check */ >>> + if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { >>> + ret = -1; >>> + goto bail; >>> + } >> >> What's the difference by moving full check above? >> As I commented in v1, we may add a more check in ocfs2_check_dir_entry(). > > Thanks for your time. > > If no match is found, there is no check for de. I'm not sure if there > will be any OOB elsewhere when "de->rec_len < > OCFS2_DIR_REC_LEN(de->name_len)". > > I'm not sure what "(de - bh->b_data) + de_len > bytes - > OCFS2_DIR_REC_LEN(de->name_len)" means in V1, bh->b_data is the start > offset of ocfs2_dinode. bytes is just the size of the de list. Maybe > we should check "((char *) de - bh->b_data) + rlen > sizeof(struct > ocfs2_dinode) + bytes" which is same as "((char *) de - bh->b_data) + > rlen > dlimit". > You can refer ext4_check_dir_entry() for details. >> >>> + >>> + if (de->name + namelen <= dlimit && >> >> As discussed in v1, this change looks reasonable. >> >> Thanks, >> Joseph >> >>> ocfs2_match(namelen, name, de)) { >>> - /* found a match - just to be sure, do a full check */ >>> - if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { >>> - ret = -1; >>> - goto bail; >>> - } >>> *res_dir = de; >>> ret = 1; >>> goto bail;
Thanks, I apologize for not being very familiar. Could you take some time to write a patch for this issue. Thanks, LL Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月29日周三 14:15写道： > > > > On 5/29/24 11:18 AM, lei lu wrote: > > Joseph Qi <joseph.qi@linux.alibaba.com> 于2024年5月29日周三 09:33写道： > >> > >> > >> > >> On 5/28/24 8:01 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 | 15 ++++++++------- > >>> 1 file changed, 8 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > >>> index d620d4c53c6f..56e2f8c62106 100644 > >>> --- a/fs/ocfs2/dir.c > >>> +++ b/fs/ocfs2/dir.c > >>> @@ -352,19 +352,20 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, > >>> de_buf = first_de; > >>> dlimit = de_buf + bytes; > >>> > >>> - while (de_buf < dlimit) { > >>> + while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { > >>> /* this code is executed quadratically often */ > >>> /* do minimal checking `by hand' */ > >>> > >>> de = (struct ocfs2_dir_entry *) de_buf; > >>> > >>> - if (de_buf + namelen <= dlimit && > >>> + /* do a full check */ > >>> + if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > >>> + ret = -1; > >>> + goto bail; > >>> + } > >> > >> What's the difference by moving full check above? > >> As I commented in v1, we may add a more check in ocfs2_check_dir_entry(). > > > > Thanks for your time. > > > > If no match is found, there is no check for de. I'm not sure if there > > will be any OOB elsewhere when "de->rec_len < > > OCFS2_DIR_REC_LEN(de->name_len)". > > > > I'm not sure what "(de - bh->b_data) + de_len > bytes - > > OCFS2_DIR_REC_LEN(de->name_len)" means in V1, bh->b_data is the start > > offset of ocfs2_dinode. bytes is just the size of the de list. Maybe > > we should check "((char *) de - bh->b_data) + rlen > sizeof(struct > > ocfs2_dinode) + bytes" which is same as "((char *) de - bh->b_data) + > > rlen > dlimit". > > > > You can refer ext4_check_dir_entry() for details. > > >> > >>> + > >>> + if (de->name + namelen <= dlimit && > >> > >> As discussed in v1, this change looks reasonable. > >> > >> Thanks, > >> Joseph > >> > >>> ocfs2_match(namelen, name, de)) { > >>> - /* found a match - just to be sure, do a full check */ > >>> - if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { > >>> - ret = -1; > >>> - goto bail; > >>> - } > >>> *res_dir = de; > >>> ret = 1; > >>> goto bail;
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index d620d4c53c6f..56e2f8c62106 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -352,19 +352,20 @@ static inline int ocfs2_search_dirblock(struct buffer_head *bh, de_buf = first_de; dlimit = de_buf + bytes; - while (de_buf < dlimit) { + while (de_buf < dlimit - OCFS2_DIR_MEMBER_LEN) { /* this code is executed quadratically often */ /* do minimal checking `by hand' */ de = (struct ocfs2_dir_entry *) de_buf; - if (de_buf + namelen <= dlimit && + /* do a full check */ + if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { + ret = -1; + goto bail; + } + + if (de->name + namelen <= dlimit && ocfs2_match(namelen, name, de)) { - /* found a match - just to be sure, do a full check */ - if (!ocfs2_check_dir_entry(dir, de, bh, offset)) { - ret = -1; - goto bail; - } *res_dir = de; 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 | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)