Received-SPF: pass (google.com: domain of linux-kernel+bounces-172574-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <tencent_6086EB12C2C654899D6EE16EF28C8727EC06@qq.com>
From: Edward Adam Davis <eadavis@qq.com>
To: kent.overstreet@linux.dev
Cc: bfoster@redhat.com,
	eadavis@qq.com,
	linux-bcachefs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	syzbot+c48865e11e7e893ec4ab@syzkaller.appspotmail.com,
	syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] bcachefs: fix oob in bch2_sb_clean_to_text
Date: Wed,  8 May 2024 08:49:39 +0800
In-Reply-To: <x73mcz4gjzdmwlynj426sq34zj232wr2z2xjbjc4tkcdbfpegb@hr55sh6pn7ol>
References: <x73mcz4gjzdmwlynj426sq34zj232wr2z2xjbjc4tkcdbfpegb@hr55sh6pn7ol>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

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;