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.0

On 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.0

On 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.