| Message ID | tencent_816D842DE96C309554E8E2ED9ACC6078120A@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bcachefs: fix oob in bch2_sb_clean_to_text | expand |
On Tue, May 07, 2024 at 05:19:29PM +0800, Edward Adam Davis wrote: > When got too small clean field, entry will never equal vstruct_end(&clean->field), > the dead loop resulted in out of bounds access. > > Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections") > Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c") > Reported-and-tested-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> I've already got a patch up for this - the validation was missing as well. commit f39055220f6f98a180e3503fe05bbf9921c425c8 Author: Kent Overstreet <kent.overstreet@linux.dev> Date: Sun May 5 22:28:00 2024 -0400 bcachefs: Add missing validation for superblock section clean We were forgetting to check for jset entries that overrun the end of the section - both in validate and to_text(); to_text() needs to be safe for types that fail to validate. Reported-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c index 35ca3f138de6..194e55b11137 100644 --- a/fs/bcachefs/sb-clean.c +++ b/fs/bcachefs/sb-clean.c @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, return -BCH_ERR_invalid_sb_clean; } + for (struct jset_entry *entry = clean->start; + entry != vstruct_end(&clean->field); + entry = vstruct_next(entry)) { + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { + prt_str(err, "entry type "); + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); + prt_str(err, " overruns end of section"); + return -BCH_ERR_invalid_sb_clean; + } + } + return 0; } @@ -295,6 +306,9 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, for (entry = clean->start; entry != vstruct_end(&clean->field); entry = vstruct_next(entry)) { + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) + break; + if (entry->type == BCH_JSET_ENTRY_btree_keys && !entry->u64s) continue;
Hi Edward, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.9-rc7 next-20240507] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635 base: linus/master patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text config: i386-buildonly-randconfig-005-20240508 (https://download.01.org/0day-ci/archive/20240508/202405080718.ZRZVFchD-lkp@intel.com/config) compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080718.ZRZVFchD-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405080718.ZRZVFchD-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/bcachefs/sb-clean.c:296:13: warning: comparison of distinct pointer types ('struct jset_entry *' and 'void *') [-Wcompare-distinct-pointer-types] 296 | entry < vstruct_end(&clean->field); | ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. vim +296 fs/bcachefs/sb-clean.c 283 284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, 285 struct bch_sb_field *f) 286 { 287 struct bch_sb_field_clean *clean = field_to_type(f, clean); 288 struct jset_entry *entry; 289 290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags)); 291 prt_newline(out); 292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq)); 293 prt_newline(out); 294 295 for (entry = clean->start; > 296 entry < vstruct_end(&clean->field); 297 entry = vstruct_next(entry)) { 298 if (entry->type == BCH_JSET_ENTRY_btree_keys && 299 !entry->u64s) 300 continue; 301 302 bch2_journal_entry_to_text(out, NULL, entry); 303 prt_newline(out); 304 } 305 } 306
Hi Edward, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.9-rc7 next-20240507] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635 base: linus/master patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text config: i386-buildonly-randconfig-004-20240508 (https://download.01.org/0day-ci/archive/20240508/202405080720.SuCCdwWG-lkp@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080720.SuCCdwWG-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405080720.SuCCdwWG-lkp@intel.com/ All warnings (new ones prefixed by >>): fs/bcachefs/sb-clean.c: In function 'bch2_sb_clean_to_text': >> fs/bcachefs/sb-clean.c:296:20: warning: comparison of distinct pointer types lacks a cast 296 | entry < vstruct_end(&clean->field); | ^ vim +296 fs/bcachefs/sb-clean.c 283 284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, 285 struct bch_sb_field *f) 286 { 287 struct bch_sb_field_clean *clean = field_to_type(f, clean); 288 struct jset_entry *entry; 289 290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags)); 291 prt_newline(out); 292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq)); 293 prt_newline(out); 294 295 for (entry = clean->start; > 296 entry < vstruct_end(&clean->field); 297 entry = vstruct_next(entry)) { 298 if (entry->type == BCH_JSET_ENTRY_btree_keys && 299 !entry->u64s) 300 continue; 301 302 bch2_journal_entry_to_text(out, NULL, entry); 303 prt_newline(out); 304 } 305 } 306
On Tue, 7 May 2024 10:14:22 -0400, Kent Overstreet wrote: > > When got too small clean field, entry will never equal vstruct_end(&clean->field), > > the dead loop resulted in out of bounds access. > > > > Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections") > > Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c") > > Reported-and-tested-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > I've already got a patch up for this - the validation was missing as > well. > > commit f39055220f6f98a180e3503fe05bbf9921c425c8 > Author: Kent Overstreet <kent.overstreet@linux.dev> > Date: Sun May 5 22:28:00 2024 -0400 > > bcachefs: Add missing validation for superblock section clean > > We were forgetting to check for jset entries that overrun the end of the > section - both in validate and to_text(); to_text() needs to be safe for > types that fail to validate. > > Reported-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c > index 35ca3f138de6..194e55b11137 100644 > --- a/fs/bcachefs/sb-clean.c > +++ b/fs/bcachefs/sb-clean.c > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, > return -BCH_ERR_invalid_sb_clean; > } > > + for (struct jset_entry *entry = clean->start; > + entry != vstruct_end(&clean->field); > + entry = vstruct_next(entry)) { > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { > + prt_str(err, "entry type "); > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); > + prt_str(err, " overruns end of section"); > + return -BCH_ERR_invalid_sb_clean; > + } > + } > + The original judgment here is sufficient, there is no need to add this section of inspection. > return 0; > } > > @@ -295,6 +306,9 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, > for (entry = clean->start; > entry != vstruct_end(&clean->field); > entry = vstruct_next(entry)) { > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) > + break; > + The same check has already been done in bch2_sb_clean_validate(), so it is unnecessary to redo it here. > if (entry->type == BCH_JSET_ENTRY_btree_keys && > !entry->u64s) > continue;
On Wed, May 08, 2024 at 08:49:39AM +0800, Edward Adam Davis wrote: > On Tue, 7 May 2024 10:14:22 -0400, Kent Overstreet wrote: > > > When got too small clean field, entry will never equal vstruct_end(&clean->field), > > > the dead loop resulted in out of bounds access. > > > > > > Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections") > > > Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c") > > > Reported-and-tested-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > > > I've already got a patch up for this - the validation was missing as > > well. > > > > commit f39055220f6f98a180e3503fe05bbf9921c425c8 > > Author: Kent Overstreet <kent.overstreet@linux.dev> > > Date: Sun May 5 22:28:00 2024 -0400 > > > > bcachefs: Add missing validation for superblock section clean > > > > We were forgetting to check for jset entries that overrun the end of the > > section - both in validate and to_text(); to_text() needs to be safe for > > types that fail to validate. > > > > Reported-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c > > index 35ca3f138de6..194e55b11137 100644 > > --- a/fs/bcachefs/sb-clean.c > > +++ b/fs/bcachefs/sb-clean.c > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, > > return -BCH_ERR_invalid_sb_clean; > > } > > > > + for (struct jset_entry *entry = clean->start; > > + entry != vstruct_end(&clean->field); > > + entry = vstruct_next(entry)) { > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { > > + prt_str(err, "entry type "); > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); > > + prt_str(err, " overruns end of section"); > > + return -BCH_ERR_invalid_sb_clean; > > + } > > + } > > + > The original judgment here is sufficient, there is no need to add this section of inspection. No, we need to be able to print things that failed to validate so that we see what went wrong.
Hi Edward, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.9-rc7 next-20240507] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Edward-Adam-Davis/bcachefs-fix-oob-in-bch2_sb_clean_to_text/20240507-172635 base: linus/master patch link: https://lore.kernel.org/r/tencent_816D842DE96C309554E8E2ED9ACC6078120A%40qq.com patch subject: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text config: x86_64-randconfig-121-20240508 (https://download.01.org/0day-ci/archive/20240508/202405080823.um76blCH-lkp@intel.com/config) compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080823.um76blCH-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405080823.um76blCH-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) fs/bcachefs/sb-clean.c: note: in included file: fs/bcachefs/bcachefs.h:1027:9: sparse: sparse: array of flexible structures fs/bcachefs/sb-clean.c: note: in included file (through fs/bcachefs/bcachefs.h): fs/bcachefs/bcachefs_format.h:794:38: sparse: sparse: array of flexible structures fs/bcachefs/bcachefs_format.h:1453:38: sparse: sparse: array of flexible structures >> fs/bcachefs/sb-clean.c:296:20: sparse: sparse: incompatible types in comparison expression (different base types): fs/bcachefs/sb-clean.c:296:20: sparse: struct jset_entry * fs/bcachefs/sb-clean.c:296:20: sparse: void * vim +296 fs/bcachefs/sb-clean.c 283 284 static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, 285 struct bch_sb_field *f) 286 { 287 struct bch_sb_field_clean *clean = field_to_type(f, clean); 288 struct jset_entry *entry; 289 290 prt_printf(out, "flags: %x", le32_to_cpu(clean->flags)); 291 prt_newline(out); 292 prt_printf(out, "journal_seq: %llu", le64_to_cpu(clean->journal_seq)); 293 prt_newline(out); 294 295 for (entry = clean->start; > 296 entry < vstruct_end(&clean->field); 297 entry = vstruct_next(entry)) { 298 if (entry->type == BCH_JSET_ENTRY_btree_keys && 299 !entry->u64s) 300 continue; 301 302 bch2_journal_entry_to_text(out, NULL, entry); 303 prt_newline(out); 304 } 305 } 306
On Tue, 7 May 2024 20:59:14 -0400, Kent Overstreet wrote: > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c > > > index 35ca3f138de6..194e55b11137 100644 > > > --- a/fs/bcachefs/sb-clean.c > > > +++ b/fs/bcachefs/sb-clean.c > > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, > > > return -BCH_ERR_invalid_sb_clean; > > > } > > > > > > + for (struct jset_entry *entry = clean->start; > > > + entry != vstruct_end(&clean->field); > > > + entry = vstruct_next(entry)) { > > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { > > > + prt_str(err, "entry type "); > > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); > > > + prt_str(err, " overruns end of section"); > > > + return -BCH_ERR_invalid_sb_clean; > > > + } > > > + } > > > + > > The original judgment here is sufficient, there is no need to add this section of inspection. > > No, we need to be able to print things that failed to validate so that > we see what went wrong. The follow check work fine, why add above check ? 1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) { 268 prt_printf(err, "wrong size (got %zu should be %zu)", 1 vstruct_bytes(&clean->field), sizeof(*clean));
On Wed, May 08, 2024 at 09:11:37AM +0800, Edward Adam Davis wrote: > On Tue, 7 May 2024 20:59:14 -0400, Kent Overstreet wrote: > > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c > > > > index 35ca3f138de6..194e55b11137 100644 > > > > --- a/fs/bcachefs/sb-clean.c > > > > +++ b/fs/bcachefs/sb-clean.c > > > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, > > > > return -BCH_ERR_invalid_sb_clean; > > > > } > > > > > > > > + for (struct jset_entry *entry = clean->start; > > > > + entry != vstruct_end(&clean->field); > > > > + entry = vstruct_next(entry)) { > > > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { > > > > + prt_str(err, "entry type "); > > > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); > > > > + prt_str(err, " overruns end of section"); > > > > + return -BCH_ERR_invalid_sb_clean; > > > > + } > > > > + } > > > > + > > > The original judgment here is sufficient, there is no need to add this section of inspection. > > > > No, we need to be able to print things that failed to validate so that > > we see what went wrong. > The follow check work fine, why add above check ? > 1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) { > 268 prt_printf(err, "wrong size (got %zu should be %zu)", > 1 vstruct_bytes(&clean->field), sizeof(*clean)); > You sure you're not inebriated?
On Tue, 7 May 2024 21:21:16 -0400, Kent Overstreet wrote: > > > > > diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c > > > > > index 35ca3f138de6..194e55b11137 100644 > > > > > --- a/fs/bcachefs/sb-clean.c > > > > > +++ b/fs/bcachefs/sb-clean.c > > > > > @@ -278,6 +278,17 @@ static int bch2_sb_clean_validate(struct bch_sb *sb, > > > > > return -BCH_ERR_invalid_sb_clean; > > > > > } > > > > > > > > > > + for (struct jset_entry *entry = clean->start; > > > > > + entry != vstruct_end(&clean->field); > > > > > + entry = vstruct_next(entry)) { > > > > > + if ((void *) vstruct_next(entry) > vstruct_end(&clean->field)) { > > > > > + prt_str(err, "entry type "); > > > > > + bch2_prt_jset_entry_type(err, le16_to_cpu(entry->type)); > > > > > + prt_str(err, " overruns end of section"); > > > > > + return -BCH_ERR_invalid_sb_clean; > > > > > + } > > > > > + } > > > > > + > > > > The original judgment here is sufficient, there is no need to add this section of inspection. > > > > > > No, we need to be able to print things that failed to validate so that > > > we see what went wrong. > > The follow check work fine, why add above check ? > > 1 if (vstruct_bytes(&clean->field) < sizeof(*clean)) { > > 268 prt_printf(err, "wrong size (got %zu should be %zu)", > > 1 vstruct_bytes(&clean->field), sizeof(*clean)); > > > > You sure you're not inebriated? Here, is my test log, according to it, I can confirm what went wrong. [ 129.350671][ T7772] bcachefs (/dev/loop0): error validating superblock: Invalid superblock section clean: wrong size (got 8 should be 24) [ 129.350671][ T7772] clean (size 8): [ 129.350671][ T7772] flags: 0 [ 129.350671][ T7772] journal_seq: 0
diff --git a/fs/bcachefs/sb-clean.c b/fs/bcachefs/sb-clean.c index 5980ba2563fe..02101687853e 100644 --- a/fs/bcachefs/sb-clean.c +++ b/fs/bcachefs/sb-clean.c @@ -285,7 +285,7 @@ static void bch2_sb_clean_to_text(struct printbuf *out, struct bch_sb *sb, prt_newline(out); for (entry = clean->start; - entry != vstruct_end(&clean->field); + entry < vstruct_end(&clean->field); entry = vstruct_next(entry)) { if (entry->type == BCH_JSET_ENTRY_btree_keys && !entry->u64s)
When got too small clean field, entry will never equal vstruct_end(&clean->field), the dead loop resulted in out of bounds access. Fixes: 12bf93a429c9 ("bcachefs: Add .to_text() methods for all superblock sections") Fixes: a37ad1a3aba9 ("bcachefs: sb-clean.c") Reported-and-tested-by: syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/bcachefs/sb-clean.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)