linux-kernel - Re: [PATCH] bcachefs: fix oob in bch2_sb_clean_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 7 May 2024 20:59:14 -0400
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Edward Adam Davis <eadavis@...com>
Cc: bfoster@...hat.com, linux-bcachefs@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzbot+c48865e11e7e893ec4ab@...kaller.appspotmail.com, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text

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@...kaller.appspotmail.com
> > > Signed-off-by: Edward Adam Davis <eadavis@...com>
> > 
> > I've already got a patch up for this - the validation was missing as
> > well.
> > 
> > commit f39055220f6f98a180e3503fe05bbf9921c425c8
> > Author: Kent Overstreet <kent.overstreet@...ux.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@...kaller.appspotmail.com
> >     Signed-off-by: Kent Overstreet <kent.overstreet@...ux.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.