fs/bcachefs/journal.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
When journal v2 entry nr overflow, it will cause the value of ja->nr to
be incorrect, this will result in the allocated memory to ja->buckets
being too small, leading to out of bounds access.
Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=47ecc948aadfb2ab3efc
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
fs/bcachefs/journal.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 13669dd0e375..d4fb5a23b3f6 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1307,8 +1307,18 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
if (journal_buckets_v2) {
unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
- for (unsigned i = 0; i < nr; i++)
+ for (unsigned i = 0; i < nr; i++) {
ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
+ if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
+ struct bch_fs *c = ca->fs;
+ struct printbuf buf = PRINTBUF;
+ prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
+ le64_to_cpu(journal_buckets_v2->d[i].nr));
+ bch_info(c, "%s", buf.buf);
+ printbuf_exit(&buf);
+ return -BCH_ERR_ENOMEM_dev_journal_init;
+ }
+ }
} else if (journal_buckets) {
ja->nr = bch2_nr_journal_buckets(journal_buckets);
}
--
2.43.0Hi Lizhi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc4 next-20240819]
[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/Lizhi-Xu/bcachefs-Fix-oob-in-bch2_dev_journal_init/20240819-145031
base: linus/master
patch link: https://lore.kernel.org/r/20240819064754.35606-1-lizhi.xu%40windriver.com
patch subject: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init
config: arc-randconfig-001-20240819 (https://download.01.org/0day-ci/archive/20240820/202408200353.I1MmR4S5-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240820/202408200353.I1MmR4S5-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/202408200353.I1MmR4S5-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from fs/bcachefs/vstructs.h:5,
from fs/bcachefs/bcachefs_format.h:80,
from fs/bcachefs/bcachefs.h:207,
from fs/bcachefs/journal.c:8:
fs/bcachefs/journal.c: In function 'bch2_dev_journal_init':
>> fs/bcachefs/journal.c:1316:50: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'long long unsigned int' [-Wformat=]
1316 | prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/bcachefs/util.h:78:63: note: in definition of macro 'prt_printf'
78 | #define prt_printf(_out, ...) bch2_prt_printf(_out, __VA_ARGS__)
| ^~~~~~~~~~~
fs/bcachefs/journal.c:1316:79: note: format string is defined here
1316 | prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
| ~~^
| |
| long unsigned int
| %llu
vim +1316 fs/bcachefs/journal.c
1297
1298 int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
1299 {
1300 struct journal_device *ja = &ca->journal;
1301 struct bch_sb_field_journal *journal_buckets =
1302 bch2_sb_field_get(sb, journal);
1303 struct bch_sb_field_journal_v2 *journal_buckets_v2 =
1304 bch2_sb_field_get(sb, journal_v2);
1305
1306 ja->nr = 0;
1307
1308 if (journal_buckets_v2) {
1309 unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
1310
1311 for (unsigned i = 0; i < nr; i++) {
1312 ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
1313 if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
1314 struct bch_fs *c = ca->fs;
1315 struct printbuf buf = PRINTBUF;
> 1316 prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
1317 le64_to_cpu(journal_buckets_v2->d[i].nr));
1318 bch_info(c, "%s", buf.buf);
1319 printbuf_exit(&buf);
1320 return -BCH_ERR_ENOMEM_dev_journal_init;
1321 }
1322 }
1323 } else if (journal_buckets) {
1324 ja->nr = bch2_nr_journal_buckets(journal_buckets);
1325 }
1326
1327 ja->bucket_seq = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
1328 if (!ja->bucket_seq)
1329 return -BCH_ERR_ENOMEM_dev_journal_init;
1330
1331 unsigned nr_bvecs = DIV_ROUND_UP(JOURNAL_ENTRY_SIZE_MAX, PAGE_SIZE);
1332
1333 for (unsigned i = 0; i < ARRAY_SIZE(ja->bio); i++) {
1334 ja->bio[i] = kmalloc(struct_size(ja->bio[i], bio.bi_inline_vecs,
1335 nr_bvecs), GFP_KERNEL);
1336 if (!ja->bio[i])
1337 return -BCH_ERR_ENOMEM_dev_journal_init;
1338
1339 ja->bio[i]->ca = ca;
1340 ja->bio[i]->buf_idx = i;
1341 bio_init(&ja->bio[i]->bio, NULL, ja->bio[i]->bio.bi_inline_vecs, nr_bvecs, 0);
1342 }
1343
1344 ja->buckets = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
1345 if (!ja->buckets)
1346 return -BCH_ERR_ENOMEM_dev_journal_init;
1347
1348 if (journal_buckets_v2) {
1349 unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
1350 unsigned dst = 0;
1351
1352 for (unsigned i = 0; i < nr; i++)
1353 for (unsigned j = 0; j < le64_to_cpu(journal_buckets_v2->d[i].nr); j++)
1354 ja->buckets[dst++] =
1355 le64_to_cpu(journal_buckets_v2->d[i].start) + j;
1356 } else if (journal_buckets) {
1357 for (unsigned i = 0; i < ja->nr; i++)
1358 ja->buckets[i] = le64_to_cpu(journal_buckets->buckets[i]);
1359 }
1360
1361 return 0;
1362 }
1363
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Lizhi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc4 next-20240819]
[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/Lizhi-Xu/bcachefs-Fix-oob-in-bch2_dev_journal_init/20240819-145031
base: linus/master
patch link: https://lore.kernel.org/r/20240819064754.35606-1-lizhi.xu%40windriver.com
patch subject: [PATCH] bcachefs: Fix oob in bch2_dev_journal_init
config: arm-randconfig-003-20240819 (https://download.01.org/0day-ci/archive/20240819/202408192244.CGhqCzQ3-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240819/202408192244.CGhqCzQ3-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/202408192244.CGhqCzQ3-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> fs/bcachefs/journal.c:1317:6: warning: format specifies type 'unsigned long' but the argument has type '__u64' (aka 'unsigned long long') [-Wformat]
1316 | prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| %llu
1317 | le64_to_cpu(journal_buckets_v2->d[i].nr));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/byteorder/generic.h:87:21: note: expanded from macro 'le64_to_cpu'
87 | #define le64_to_cpu __le64_to_cpu
| ^
include/uapi/linux/byteorder/little_endian.h:33:26: note: expanded from macro '__le64_to_cpu'
33 | #define __le64_to_cpu(x) ((__force __u64)(__le64)(x))
| ^
fs/bcachefs/util.h:78:54: note: expanded from macro 'prt_printf'
78 | #define prt_printf(_out, ...) bch2_prt_printf(_out, __VA_ARGS__)
| ^~~~~~~~~~~
1 warning generated.
vim +1317 fs/bcachefs/journal.c
1297
1298 int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
1299 {
1300 struct journal_device *ja = &ca->journal;
1301 struct bch_sb_field_journal *journal_buckets =
1302 bch2_sb_field_get(sb, journal);
1303 struct bch_sb_field_journal_v2 *journal_buckets_v2 =
1304 bch2_sb_field_get(sb, journal_v2);
1305
1306 ja->nr = 0;
1307
1308 if (journal_buckets_v2) {
1309 unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
1310
1311 for (unsigned i = 0; i < nr; i++) {
1312 ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
1313 if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
1314 struct bch_fs *c = ca->fs;
1315 struct printbuf buf = PRINTBUF;
1316 prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
> 1317 le64_to_cpu(journal_buckets_v2->d[i].nr));
1318 bch_info(c, "%s", buf.buf);
1319 printbuf_exit(&buf);
1320 return -BCH_ERR_ENOMEM_dev_journal_init;
1321 }
1322 }
1323 } else if (journal_buckets) {
1324 ja->nr = bch2_nr_journal_buckets(journal_buckets);
1325 }
1326
1327 ja->bucket_seq = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
1328 if (!ja->bucket_seq)
1329 return -BCH_ERR_ENOMEM_dev_journal_init;
1330
1331 unsigned nr_bvecs = DIV_ROUND_UP(JOURNAL_ENTRY_SIZE_MAX, PAGE_SIZE);
1332
1333 for (unsigned i = 0; i < ARRAY_SIZE(ja->bio); i++) {
1334 ja->bio[i] = kmalloc(struct_size(ja->bio[i], bio.bi_inline_vecs,
1335 nr_bvecs), GFP_KERNEL);
1336 if (!ja->bio[i])
1337 return -BCH_ERR_ENOMEM_dev_journal_init;
1338
1339 ja->bio[i]->ca = ca;
1340 ja->bio[i]->buf_idx = i;
1341 bio_init(&ja->bio[i]->bio, NULL, ja->bio[i]->bio.bi_inline_vecs, nr_bvecs, 0);
1342 }
1343
1344 ja->buckets = kcalloc(ja->nr, sizeof(u64), GFP_KERNEL);
1345 if (!ja->buckets)
1346 return -BCH_ERR_ENOMEM_dev_journal_init;
1347
1348 if (journal_buckets_v2) {
1349 unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
1350 unsigned dst = 0;
1351
1352 for (unsigned i = 0; i < nr; i++)
1353 for (unsigned j = 0; j < le64_to_cpu(journal_buckets_v2->d[i].nr); j++)
1354 ja->buckets[dst++] =
1355 le64_to_cpu(journal_buckets_v2->d[i].start) + j;
1356 } else if (journal_buckets) {
1357 for (unsigned i = 0; i < ja->nr; i++)
1358 ja->buckets[i] = le64_to_cpu(journal_buckets->buckets[i]);
1359 }
1360
1361 return 0;
1362 }
1363
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, Aug 19, 2024 at 02:47:54PM GMT, Lizhi Xu wrote:
> When journal v2 entry nr overflow, it will cause the value of ja->nr to
> be incorrect, this will result in the allocated memory to ja->buckets
> being too small, leading to out of bounds access.
>
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=47ecc948aadfb2ab3efc
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> fs/bcachefs/journal.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> index 13669dd0e375..d4fb5a23b3f6 100644
> --- a/fs/bcachefs/journal.c
> +++ b/fs/bcachefs/journal.c
> @@ -1307,8 +1307,18 @@ int bch2_dev_journal_init(struct bch_dev *ca, struct bch_sb *sb)
> if (journal_buckets_v2) {
> unsigned nr = bch2_sb_field_journal_v2_nr_entries(journal_buckets_v2);
>
> - for (unsigned i = 0; i < nr; i++)
> + for (unsigned i = 0; i < nr; i++) {
> ja->nr += le64_to_cpu(journal_buckets_v2->d[i].nr);
> + if (le64_to_cpu(journal_buckets_v2->d[i].nr) > UINT_MAX) {
> + struct bch_fs *c = ca->fs;
> + struct printbuf buf = PRINTBUF;
> + prt_printf(&buf, "journal v2 entry d[%u].nr %lu overflow!\n", i,
> + le64_to_cpu(journal_buckets_v2->d[i].nr));
> + bch_info(c, "%s", buf.buf);
> + printbuf_exit(&buf);
> + return -BCH_ERR_ENOMEM_dev_journal_init;
> + }
> + }
this should be done in the validation code
When journal v2 entry nr overflow, it will cause the value of ja->nr to
be incorrect, this will result in the allocated memory to ja->buckets
being too small, leading to out of bounds access in bch2_dev_journal_init.
Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
fs/bcachefs/journal_sb.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
index db80e506e3ab..db2b2100e4e5 100644
--- a/fs/bcachefs/journal_sb.c
+++ b/fs/bcachefs/journal_sb.c
@@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
for (i = 0; i < nr; i++) {
b[i].start = le64_to_cpu(journal->d[i].start);
b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
+ if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
+ prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
+ i, le64_to_cpu(journal->d[i].nr));
+ goto err;
+ }
}
sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
--
2.43.0On Tue, Aug 20, 2024 at 03:11:45PM GMT, Lizhi Xu wrote:
> When journal v2 entry nr overflow, it will cause the value of ja->nr to
> be incorrect, this will result in the allocated memory to ja->buckets
> being too small, leading to out of bounds access in bch2_dev_journal_init.
>
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> fs/bcachefs/journal_sb.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> index db80e506e3ab..db2b2100e4e5 100644
> --- a/fs/bcachefs/journal_sb.c
> +++ b/fs/bcachefs/journal_sb.c
> @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> for (i = 0; i < nr; i++) {
> b[i].start = le64_to_cpu(journal->d[i].start);
> b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> + if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> + prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> + i, le64_to_cpu(journal->d[i].nr));
> + goto err;
> + }
no, you need to sum up _all_ the entries and verify the total doesn't
overflow UINT_MAX
On Tue, 20 Aug 2024 19:34:18 -0400, Kent Overstreet wrote:
> > When journal v2 entry nr overflow, it will cause the value of ja->nr to
> > be incorrect, this will result in the allocated memory to ja->buckets
> > being too small, leading to out of bounds access in bch2_dev_journal_init.
> >
> > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> > fs/bcachefs/journal_sb.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > index db80e506e3ab..db2b2100e4e5 100644
> > --- a/fs/bcachefs/journal_sb.c
> > +++ b/fs/bcachefs/journal_sb.c
> > @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > for (i = 0; i < nr; i++) {
> > b[i].start = le64_to_cpu(journal->d[i].start);
> > b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > + if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> > + prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> > + i, le64_to_cpu(journal->d[i].nr));
> > + goto err;
> > + }
>
> no, you need to sum up _all_ the entries and verify the total doesn't
> overflow UINT_MAX
The overflow value of le64_to_cpu(journal->d[i].nr) is 18446744073709551615(for u64),
or in other words, it is -1 for s64.
Therefore, the existing check for single entry is retained, and an overflow
check for the total value of all entry is will added.
BR,
Lizhi
On Wed, Aug 21, 2024 at 10:33:55AM GMT, Lizhi Xu wrote:
> On Tue, 20 Aug 2024 19:34:18 -0400, Kent Overstreet wrote:
> > > When journal v2 entry nr overflow, it will cause the value of ja->nr to
> > > be incorrect, this will result in the allocated memory to ja->buckets
> > > being too small, leading to out of bounds access in bch2_dev_journal_init.
> > >
> > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > > fs/bcachefs/journal_sb.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > index db80e506e3ab..db2b2100e4e5 100644
> > > --- a/fs/bcachefs/journal_sb.c
> > > +++ b/fs/bcachefs/journal_sb.c
> > > @@ -119,6 +119,11 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > for (i = 0; i < nr; i++) {
> > > b[i].start = le64_to_cpu(journal->d[i].start);
> > > b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > > + if (le64_to_cpu(journal->d[i].nr) > UINT_MAX) {
> > > + prt_printf(err, "journal v2 entry d[%u].nr %llu overflow\n",
> > > + i, le64_to_cpu(journal->d[i].nr));
> > > + goto err;
> > > + }
> >
> > no, you need to sum up _all_ the entries and verify the total doesn't
> > overflow UINT_MAX
> The overflow value of le64_to_cpu(journal->d[i].nr) is 18446744073709551615(for u64),
> or in other words, it is -1 for s64.
>
> Therefore, the existing check for single entry is retained, and an overflow
> check for the total value of all entry is will added.
No, this is completely broken.
When the nr value of a signle entry or their sum overflows, it will
cause the value of ja->nr to be incorrect, this will result in the
allocated memory to ja->buckets being too small, leading to out of
bounds access in bch2_dev_journal_init.
Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
index db80e506e3ab..230ed99130e4 100644
--- a/fs/bcachefs/journal_sb.c
+++ b/fs/bcachefs/journal_sb.c
@@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
unsigned nr;
unsigned i;
struct u64_range *b;
+ u64 total_nr = 0, entry_nr;
nr = bch2_sb_field_journal_v2_nr_entries(journal);
if (!nr)
@@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
for (i = 0; i < nr; i++) {
+ entry_nr = le64_to_cpu(journal->d[i].nr);
+ if (entry_nr > UINT_MAX) {
+ prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
+ i, entry_nr);
+ goto err;
+ }
+ total_nr += entry_nr;
b[i].start = le64_to_cpu(journal->d[i].start);
- b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
+ b[i].end = b[i].start + entry_nr;
+ }
+
+ if (total_nr > UINT_MAX) {
+ prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
+ total_nr);
+ goto err;
}
sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
--
2.43.0On Wed, Aug 21, 2024 at 10:57:37AM GMT, Lizhi Xu wrote:
> When the nr value of a signle entry or their sum overflows, it will
> cause the value of ja->nr to be incorrect, this will result in the
> allocated memory to ja->buckets being too small, leading to out of
> bounds access in bch2_dev_journal_init.
>
> Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> index db80e506e3ab..230ed99130e4 100644
> --- a/fs/bcachefs/journal_sb.c
> +++ b/fs/bcachefs/journal_sb.c
> @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> unsigned nr;
> unsigned i;
> struct u64_range *b;
> + u64 total_nr = 0, entry_nr;
>
> nr = bch2_sb_field_journal_v2_nr_entries(journal);
> if (!nr)
> @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
>
> for (i = 0; i < nr; i++) {
> + entry_nr = le64_to_cpu(journal->d[i].nr);
> + if (entry_nr > UINT_MAX) {
> + prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> + i, entry_nr);
> + goto err;
> + }
This check is unnecessary; we know the sum can't overflow a u64 because
we're also checking that the entries are nonoverlapping.
> + total_nr += entry_nr;
> b[i].start = le64_to_cpu(journal->d[i].start);
> - b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> + b[i].end = b[i].start + entry_nr;
> + }
> +
> + if (total_nr > UINT_MAX) {
> + prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
> + total_nr);
> + goto err;
> }
>
> sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
> --
> 2.43.0
>
On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > When the nr value of a signle entry or their sum overflows, it will
> > cause the value of ja->nr to be incorrect, this will result in the
> > allocated memory to ja->buckets being too small, leading to out of
> > bounds access in bch2_dev_journal_init.
> >
> > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> > fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > index db80e506e3ab..230ed99130e4 100644
> > --- a/fs/bcachefs/journal_sb.c
> > +++ b/fs/bcachefs/journal_sb.c
> > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > unsigned nr;
> > unsigned i;
> > struct u64_range *b;
> > + u64 total_nr = 0, entry_nr;
> >
> > nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > if (!nr)
> > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> >
> > for (i = 0; i < nr; i++) {
> > + entry_nr = le64_to_cpu(journal->d[i].nr);
> > + if (entry_nr > UINT_MAX) {
> > + prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > + i, entry_nr);
> > + goto err;
> > + }
>
> This check is unnecessary; we know the sum can't overflow a u64 because
> we're also checking that the entries are nonoverlapping.
You didn't read my previous email carefully.
In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
so the sum of their u64 type values will definitely overflow.
>
> > + total_nr += entry_nr;
> > b[i].start = le64_to_cpu(journal->d[i].start);
> > - b[i].end = b[i].start + le64_to_cpu(journal->d[i].nr);
> > + b[i].end = b[i].start + entry_nr;
> > + }
> > +
> > + if (total_nr > UINT_MAX) {
> > + prt_printf(err, "Sum of journal v2 entries nr %llu overflow\n",
> > + total_nr);
> > + goto err;
> > }
> >
> > sort(b, nr, sizeof(*b), u64_range_cmp, NULL);
> > --
BR,
Lizhi
On Wed, Aug 21, 2024 at 11:10:00AM GMT, Lizhi Xu wrote:
> On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > When the nr value of a signle entry or their sum overflows, it will
> > > cause the value of ja->nr to be incorrect, this will result in the
> > > allocated memory to ja->buckets being too small, leading to out of
> > > bounds access in bch2_dev_journal_init.
> > >
> > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > > fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > > 1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > index db80e506e3ab..230ed99130e4 100644
> > > --- a/fs/bcachefs/journal_sb.c
> > > +++ b/fs/bcachefs/journal_sb.c
> > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > unsigned nr;
> > > unsigned i;
> > > struct u64_range *b;
> > > + u64 total_nr = 0, entry_nr;
> > >
> > > nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > > if (!nr)
> > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > >
> > > for (i = 0; i < nr; i++) {
> > > + entry_nr = le64_to_cpu(journal->d[i].nr);
> > > + if (entry_nr > UINT_MAX) {
> > > + prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > + i, entry_nr);
> > > + goto err;
> > > + }
> >
> > This check is unnecessary; we know the sum can't overflow a u64 because
> > we're also checking that the entries are nonoverlapping.
> You didn't read my previous email carefully.
> In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> so the sum of their u64 type values will definitely overflow.
It doesn't matter. We're already checking that the entries are
nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
overflow nbuckets, much less an s64 (not that that matters).
On Tue, 20 Aug 2024 23:16:50 -0400, Lizhi Xu wrote:
> > On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > > When the nr value of a signle entry or their sum overflows, it will
> > > > cause the value of ja->nr to be incorrect, this will result in the
> > > > allocated memory to ja->buckets being too small, leading to out of
> > > > bounds access in bch2_dev_journal_init.
> > > >
> > > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > ---
> > > > fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > > > 1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > > index db80e506e3ab..230ed99130e4 100644
> > > > --- a/fs/bcachefs/journal_sb.c
> > > > +++ b/fs/bcachefs/journal_sb.c
> > > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > > unsigned nr;
> > > > unsigned i;
> > > > struct u64_range *b;
> > > > + u64 total_nr = 0, entry_nr;
> > > >
> > > > nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > > > if (!nr)
> > > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > > return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > > >
> > > > for (i = 0; i < nr; i++) {
> > > > + entry_nr = le64_to_cpu(journal->d[i].nr);
> > > > + if (entry_nr > UINT_MAX) {
> > > > + prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > > + i, entry_nr);
> > > > + goto err;
> > > > + }
> > >
> > > This check is unnecessary; we know the sum can't overflow a u64 because
> > > we're also checking that the entries are nonoverlapping.
> > You didn't read my previous email carefully.
> > In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> > so the sum of their u64 type values will definitely overflow.
>
> It doesn't matter. We're already checking that the entries are
> nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
> overflow nbuckets, much less an s64 (not that that matters).
Are you sure? Or did I not express myself clearly? See the actual running results below:
le64_to_cpu(journal->d[1].nr) + le64_to_cpu(journal->d[0].nr) = 7 + 18446744073709551615 will overflow.
When a u64 overflow occurs, total_nr will not be greater than UINT_MAX,
so it is not enough to only calculate the total nr of entry to determine.
Note: u64 contains 0 ~ 18446744073709551616.
BR,
Lizhi
On Tue, Aug 20, 2024 at 11:16:55PM GMT, Kent Overstreet wrote:
> On Wed, Aug 21, 2024 at 11:10:00AM GMT, Lizhi Xu wrote:
> > On Tue, 20 Aug 2024 23:00:05 -0400, Kent Overstreet wrote:
> > > > When the nr value of a signle entry or their sum overflows, it will
> > > > cause the value of ja->nr to be incorrect, this will result in the
> > > > allocated memory to ja->buckets being too small, leading to out of
> > > > bounds access in bch2_dev_journal_init.
> > > >
> > > > Reported-by: syzbot+47ecc948aadfb2ab3efc@syzkaller.appspotmail.com
> > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > ---
> > > > fs/bcachefs/journal_sb.c | 16 +++++++++++++++-
> > > > 1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/bcachefs/journal_sb.c b/fs/bcachefs/journal_sb.c
> > > > index db80e506e3ab..230ed99130e4 100644
> > > > --- a/fs/bcachefs/journal_sb.c
> > > > +++ b/fs/bcachefs/journal_sb.c
> > > > @@ -107,6 +107,7 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > > unsigned nr;
> > > > unsigned i;
> > > > struct u64_range *b;
> > > > + u64 total_nr = 0, entry_nr;
> > > >
> > > > nr = bch2_sb_field_journal_v2_nr_entries(journal);
> > > > if (!nr)
> > > > @@ -117,8 +118,21 @@ static int bch2_sb_journal_v2_validate(struct bch_sb *sb, struct bch_sb_field *f
> > > > return -BCH_ERR_ENOMEM_sb_journal_v2_validate;
> > > >
> > > > for (i = 0; i < nr; i++) {
> > > > + entry_nr = le64_to_cpu(journal->d[i].nr);
> > > > + if (entry_nr > UINT_MAX) {
> > > > + prt_printf(err, "Journal v2 entry d[%u] nr %llu overflow\n",
> > > > + i, entry_nr);
> > > > + goto err;
> > > > + }
> > >
> > > This check is unnecessary; we know the sum can't overflow a u64 because
> > > we're also checking that the entries are nonoverlapping.
> > You didn't read my previous email carefully.
> > In this issue, journal->d[0] is 7, journal->d[1] is 18446744073709551615,
> > so the sum of their u64 type values will definitely overflow.
>
> It doesn't matter. We're already checking that the entries are
> nonoverlapping, and within the range of [1, nbuckets), so total_nr can't
> overflow nbuckets, much less an s64 (not that that matters).
The check that's missing is that start + nr doesn't overflow, when we
convert to u64_ranges.
© 2016 - 2026 Red Hat, Inc.