* [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() @ 2025-06-27 2:38 Ivan Pravdin 2025-06-30 1:26 ` Joseph Qi 0 siblings, 1 reply; 7+ messages in thread From: Ivan Pravdin @ 2025-06-27 2:38 UTC (permalink / raw) To: mark, jlbec, joseph.qi, ocfs2-devel, linux-kernel Cc: Ivan Pravdin, syzbot+20282c1b2184a857ac4c When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an error message that unconditionally dereferences the 'rec' pointer. However, if 'rec' is NULL, this leads to a NULL pointer dereference and a kernel panic. Add an explicit check for a NULL 'rec' and use an alternate error message in that case to avoid unsafe access. Reported-by: syzbot+20282c1b2184a857ac4c@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ Signed-off-by: Ivan Pravdin <ipravdin.official@gmail.com> --- fs/ocfs2/dir.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index 7799f4d16ce9..dccf0349e523 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -809,11 +809,17 @@ static int ocfs2_dx_dir_lookup_rec(struct inode *inode, } if (!found) { - ret = ocfs2_error(inode->i_sb, - "Inode %lu has bad extent record (%u, %u, 0) in btree\n", - inode->i_ino, - le32_to_cpu(rec->e_cpos), - ocfs2_rec_clusters(el, rec)); + if (rec) { + ret = ocfs2_error(inode->i_sb, + "Inode %lu has bad extent record (%u, %u, 0) in btree\n", + inode->i_ino, + le32_to_cpu(rec->e_cpos), + ocfs2_rec_clusters(el, rec)); + } else { + ret = ocfs2_error(inode->i_sb, + "Inode %lu has no extent records in btree\n", + inode->i_ino); + } goto out; } -- 2.45.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-27 2:38 [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() Ivan Pravdin @ 2025-06-30 1:26 ` Joseph Qi 2025-06-30 2:32 ` Heming Zhao 0 siblings, 1 reply; 7+ messages in thread From: Joseph Qi @ 2025-06-30 1:26 UTC (permalink / raw) To: Ivan Pravdin, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c Hi, On 2025/6/27 10:38, Ivan Pravdin wrote: > When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an > error message that unconditionally dereferences the 'rec' pointer. > However, if 'rec' is NULL, this leads to a NULL pointer dereference and > a kernel panic. > This looks possible, but syzbot reports slab-out-of-bounds Read in ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. So I think it is because it construct a malicious image and set a wrong l_recs, then access this damaged l_recs. Thanks, Joseph > Add an explicit check for a NULL 'rec' and use an alternate error > message in that case to avoid unsafe access. > > Reported-by: syzbot+20282c1b2184a857ac4c@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ > Signed-off-by: Ivan Pravdin <ipravdin.official@gmail.com> > --- > fs/ocfs2/dir.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index 7799f4d16ce9..dccf0349e523 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -809,11 +809,17 @@ static int ocfs2_dx_dir_lookup_rec(struct inode *inode, > } > > if (!found) { > - ret = ocfs2_error(inode->i_sb, > - "Inode %lu has bad extent record (%u, %u, 0) in btree\n", > - inode->i_ino, > - le32_to_cpu(rec->e_cpos), > - ocfs2_rec_clusters(el, rec)); > + if (rec) { > + ret = ocfs2_error(inode->i_sb, > + "Inode %lu has bad extent record (%u, %u, 0) in btree\n", > + inode->i_ino, > + le32_to_cpu(rec->e_cpos), > + ocfs2_rec_clusters(el, rec)); > + } else { > + ret = ocfs2_error(inode->i_sb, > + "Inode %lu has no extent records in btree\n", > + inode->i_ino); > + } > goto out; > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-30 1:26 ` Joseph Qi @ 2025-06-30 2:32 ` Heming Zhao 2025-06-30 2:45 ` Ivan Pravdin 2025-06-30 6:58 ` Joseph Qi 0 siblings, 2 replies; 7+ messages in thread From: Heming Zhao @ 2025-06-30 2:32 UTC (permalink / raw) To: Joseph Qi, Ivan Pravdin, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c On 6/30/25 09:26, Joseph Qi wrote: > Hi, > > > On 2025/6/27 10:38, Ivan Pravdin wrote: >> When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an >> error message that unconditionally dereferences the 'rec' pointer. >> However, if 'rec' is NULL, this leads to a NULL pointer dereference and >> a kernel panic. >> > > This looks possible, but syzbot reports slab-out-of-bounds Read in > ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. > > So I think it is because it construct a malicious image and set a wrong > l_recs, then access this damaged l_recs. > > Thanks, > Joseph I think this proposed fix (at least the fix method) is acceptable. the crash occurs at ocfs2_error(), where the pointer 'rec' must be incorrect. look back at the previous code lines: for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) { rec = &el->l_recs[i]; if (le32_to_cpu(rec->e_cpos) <= major_hash) { found = 1; break; } } either 'el->l_next_free_rec' or 'el->l_recs[i]' has an incorrect value. we can do nothing about this kind of error, simply returning errno to caller is sufficient. btw, ocfs2-tools has a similar function "static errcode_t ocfs2_dx_dir_lookup_rec()"@libocfs2/dir_indexed.c, which sets ret with OCFS2_ET_CORRUPT_EXTENT_BLOCK and return. - Heming > > >> Add an explicit check for a NULL 'rec' and use an alternate error >> message in that case to avoid unsafe access. >> >> Reported-by: syzbot+20282c1b2184a857ac4c@syzkaller.appspotmail.com >> Closes: https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ >> Signed-off-by: Ivan Pravdin <ipravdin.official@gmail.com> >> --- >> fs/ocfs2/dir.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >> index 7799f4d16ce9..dccf0349e523 100644 >> --- a/fs/ocfs2/dir.c >> +++ b/fs/ocfs2/dir.c >> @@ -809,11 +809,17 @@ static int ocfs2_dx_dir_lookup_rec(struct inode *inode, >> } >> >> if (!found) { >> - ret = ocfs2_error(inode->i_sb, >> - "Inode %lu has bad extent record (%u, %u, 0) in btree\n", >> - inode->i_ino, >> - le32_to_cpu(rec->e_cpos), >> - ocfs2_rec_clusters(el, rec)); >> + if (rec) { >> + ret = ocfs2_error(inode->i_sb, >> + "Inode %lu has bad extent record (%u, %u, 0) in btree\n", >> + inode->i_ino, >> + le32_to_cpu(rec->e_cpos), >> + ocfs2_rec_clusters(el, rec)); >> + } else { >> + ret = ocfs2_error(inode->i_sb, >> + "Inode %lu has no extent records in btree\n", >> + inode->i_ino); >> + } >> goto out; >> } >> > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-30 2:32 ` Heming Zhao @ 2025-06-30 2:45 ` Ivan Pravdin 2025-06-30 3:40 ` Ivan Pravdin 2025-06-30 6:58 ` Joseph Qi 1 sibling, 1 reply; 7+ messages in thread From: Ivan Pravdin @ 2025-06-30 2:45 UTC (permalink / raw) To: Heming Zhao, Joseph Qi, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c On Mon, Jun 30, 2025 at 10:32:35AM GMT, Heming Zhao wrote: > On 6/30/25 09:26, Joseph Qi wrote: > > Hi, > > > > > > On 2025/6/27 10:38, Ivan Pravdin wrote: > > > When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an > > > error message that unconditionally dereferences the 'rec' pointer. > > > However, if 'rec' is NULL, this leads to a NULL pointer dereference and > > > a kernel panic. > > > > > > > This looks possible, but syzbot reports slab-out-of-bounds Read in > > ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. > > > > So I think it is because it construct a malicious image and set a wrong > > l_recs, then access this damaged l_recs. > > > > Thanks, > > Joseph > > I think this proposed fix (at least the fix method) is acceptable. > the crash occurs at ocfs2_error(), where the pointer 'rec' must be incorrect. > look back at the previous code lines: > for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) { > rec = &el->l_recs[i]; > > if (le32_to_cpu(rec->e_cpos) <= major_hash) { > found = 1; > break; > } > } > > either 'el->l_next_free_rec' or 'el->l_recs[i]' has an incorrect value. > we can do nothing about this kind of error, simply returning errno to caller is sufficient. > > btw, ocfs2-tools has a similar function "static errcode_t ocfs2_dx_dir_lookup_rec()"@libocfs2/dir_indexed.c, which sets ret with OCFS2_ET_CORRUPT_EXTENT_BLOCK and return. Thanks, I will include it in v2. > > - Heming > > > > > > > > Add an explicit check for a NULL 'rec' and use an alternate error > > > message in that case to avoid unsafe access. > > > > > > Reported-by: syzbot+20282c1b2184a857ac4c@syzkaller.appspotmail.com > > > Closes: https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ > > > Signed-off-by: Ivan Pravdin <ipravdin.official@gmail.com> > > > --- > > > fs/ocfs2/dir.c | 16 +++++++++++----- > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > > > index 7799f4d16ce9..dccf0349e523 100644 > > > --- a/fs/ocfs2/dir.c > > > +++ b/fs/ocfs2/dir.c > > > @@ -809,11 +809,17 @@ static int ocfs2_dx_dir_lookup_rec(struct inode *inode, > > > } > > > if (!found) { > > > - ret = ocfs2_error(inode->i_sb, > > > - "Inode %lu has bad extent record (%u, %u, 0) in btree\n", > > > - inode->i_ino, > > > - le32_to_cpu(rec->e_cpos), > > > - ocfs2_rec_clusters(el, rec)); > > > + if (rec) { > > > + ret = ocfs2_error(inode->i_sb, > > > + "Inode %lu has bad extent record (%u, %u, 0) in btree\n", > > > + inode->i_ino, > > > + le32_to_cpu(rec->e_cpos), > > > + ocfs2_rec_clusters(el, rec)); > > > + } else { > > > + ret = ocfs2_error(inode->i_sb, > > > + "Inode %lu has no extent records in btree\n", > > > + inode->i_ino); > > > + } > > > goto out; > > > } > > > > > Ivan Pravdin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-30 2:45 ` Ivan Pravdin @ 2025-06-30 3:40 ` Ivan Pravdin 0 siblings, 0 replies; 7+ messages in thread From: Ivan Pravdin @ 2025-06-30 3:40 UTC (permalink / raw) To: Heming Zhao, Joseph Qi, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c On Sun, Jun 29, 2025 at 10:45:15PM GMT, Ivan Pravdin wrote: > On Mon, Jun 30, 2025 at 10:32:35AM GMT, Heming Zhao wrote: > > On 6/30/25 09:26, Joseph Qi wrote: > > > Hi, > > > > > > > > > On 2025/6/27 10:38, Ivan Pravdin wrote: > > > > When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an > > > > error message that unconditionally dereferences the 'rec' pointer. > > > > However, if 'rec' is NULL, this leads to a NULL pointer dereference and > > > > a kernel panic. > > > > > > > > > > This looks possible, but syzbot reports slab-out-of-bounds Read in > > > ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. > > > > > > So I think it is because it construct a malicious image and set a wrong > > > l_recs, then access this damaged l_recs. > > > > > > Thanks, > > > Joseph > > > > I think this proposed fix (at least the fix method) is acceptable. > > the crash occurs at ocfs2_error(), where the pointer 'rec' must be incorrect. > > look back at the previous code lines: > > for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) { > > rec = &el->l_recs[i]; > > > > if (le32_to_cpu(rec->e_cpos) <= major_hash) { > > found = 1; > > break; > > } > > } > > > > either 'el->l_next_free_rec' or 'el->l_recs[i]' has an incorrect value. > > we can do nothing about this kind of error, simply returning errno to caller is sufficient. > > > > btw, ocfs2-tools has a similar function "static errcode_t ocfs2_dx_dir_lookup_rec()"@libocfs2/dir_indexed.c, which sets ret with OCFS2_ET_CORRUPT_EXTENT_BLOCK and return. > > Thanks, I will include it in v2. I have realized that ret is already set to EROFS that acts as a general FS errno. Unless there are any other objectives this is the latest version. Ivan Pravdin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-30 2:32 ` Heming Zhao 2025-06-30 2:45 ` Ivan Pravdin @ 2025-06-30 6:58 ` Joseph Qi 2025-06-30 7:23 ` Heming Zhao 1 sibling, 1 reply; 7+ messages in thread From: Joseph Qi @ 2025-06-30 6:58 UTC (permalink / raw) To: Heming Zhao, Ivan Pravdin, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c On 2025/6/30 10:32, Heming Zhao wrote: > On 6/30/25 09:26, Joseph Qi wrote: >> Hi, >> >> >> On 2025/6/27 10:38, Ivan Pravdin wrote: >>> When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an >>> error message that unconditionally dereferences the 'rec' pointer. >>> However, if 'rec' is NULL, this leads to a NULL pointer dereference and >>> a kernel panic. >>> >> >> This looks possible, but syzbot reports slab-out-of-bounds Read in >> ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. >> >> So I think it is because it construct a malicious image and set a wrong >> l_recs, then access this damaged l_recs. >> >> Thanks, >> Joseph > > I think this proposed fix (at least the fix method) is acceptable. But this seems different from the issue syzbot reports, right? https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ Thanks, Joseph > the crash occurs at ocfs2_error(), where the pointer 'rec' must be incorrect. > look back at the previous code lines: > for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) { > rec = &el->l_recs[i]; > > if (le32_to_cpu(rec->e_cpos) <= major_hash) { > found = 1; > break; > } > } > > either 'el->l_next_free_rec' or 'el->l_recs[i]' has an incorrect value. > we can do nothing about this kind of error, simply returning errno to caller is sufficient. > > btw, ocfs2-tools has a similar function "static errcode_t ocfs2_dx_dir_lookup_rec()"@libocfs2/dir_indexed.c, which sets ret with OCFS2_ET_CORRUPT_EXTENT_BLOCK and return. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() 2025-06-30 6:58 ` Joseph Qi @ 2025-06-30 7:23 ` Heming Zhao 0 siblings, 0 replies; 7+ messages in thread From: Heming Zhao @ 2025-06-30 7:23 UTC (permalink / raw) To: Joseph Qi, Ivan Pravdin, mark, jlbec, ocfs2-devel, linux-kernel Cc: syzbot+20282c1b2184a857ac4c On 6/30/25 14:58, Joseph Qi wrote: > > > On 2025/6/30 10:32, Heming Zhao wrote: >> On 6/30/25 09:26, Joseph Qi wrote: >>> Hi, >>> >>> >>> On 2025/6/27 10:38, Ivan Pravdin wrote: >>>> When a directory entry is not found, ocfs2_dx_dir_lookup_rec() prints an >>>> error message that unconditionally dereferences the 'rec' pointer. >>>> However, if 'rec' is NULL, this leads to a NULL pointer dereference and >>>> a kernel panic. >>>> >>> >>> This looks possible, but syzbot reports slab-out-of-bounds Read in >>> ocfs2_dx_dir_lookup_rec(), not NULL pointer dereference. >>> >>> So I think it is because it construct a malicious image and set a wrong >>> l_recs, then access this damaged l_recs. >>> >>> Thanks, >>> Joseph >> >> I think this proposed fix (at least the fix method) is acceptable. > > But this seems different from the issue syzbot reports, right? > https://lore.kernel.org/all/67483b75.050a0220.253251.007c.GAE@google.com/T/ > > Thanks, > Joseph OK, I think I may have found the problem. In the URL above, there are two issues/reports: 1> From: syzbot @ 2024-11-28 9:44 UTC 2> From: syzbot @ 2025-03-09 11:40 UTC Your review comments about report <1>; Ivan's patch is about report <2>. And the dashboard shows the <2>: - https://syzkaller.appspot.com/bug?extid=20282c1b2184a857ac4c It seems syzbot changed the fuzz code and won't trigger issue <1>. For issue <1>, the rec was an illegal value, and made KASAN report a bug. > >> the crash occurs at ocfs2_error(), where the pointer 'rec' must be incorrect. >> look back at the previous code lines: >> for (i = le16_to_cpu(el->l_next_free_rec) - 1; i >= 0; i--) { >> rec = &el->l_recs[i]; >> >> if (le32_to_cpu(rec->e_cpos) <= major_hash) { >> found = 1; >> break; >> } >> } >> >> either 'el->l_next_free_rec' or 'el->l_recs[i]' has an incorrect value. >> we can do nothing about this kind of error, simply returning errno to caller is sufficient. >> >> btw, ocfs2-tools has a similar function "static errcode_t ocfs2_dx_dir_lookup_rec()"@libocfs2/dir_indexed.c, which sets ret with OCFS2_ET_CORRUPT_EXTENT_BLOCK and return. >> > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-30 7:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-06-27 2:38 [PATCH] ocfs2: Avoid NULL pointer dereference in dx_dir_lookup_rec() Ivan Pravdin 2025-06-30 1:26 ` Joseph Qi 2025-06-30 2:32 ` Heming Zhao 2025-06-30 2:45 ` Ivan Pravdin 2025-06-30 3:40 ` Ivan Pravdin 2025-06-30 6:58 ` Joseph Qi 2025-06-30 7:23 ` Heming Zhao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®