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]
Message-ID: <7chwa5h2y2eotafxfnapxn754n7y3zpze2sm5dif3zyx7hkxcc@2zu6pskc7fbo>
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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ