[f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

[f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

[Yangtao Li via Linux-f2fs-devel]

Add iotype sanity check to avoid potential memory corruption.
This is to fix the compile error below:

fs/f2fs/iostat.c:231 __update_iostat_latency() error: buffer overflow
'io_lat->peak_lat[type]' 3 <= 3

vim +228 fs/f2fs/iostat.c

  211  static inline void __update_iostat_latency(struct bio_iostat_ctx
	*iostat_ctx,
  212					enum iostat_lat_type type)
  213  {
  214		unsigned long ts_diff;
  215		unsigned int page_type = iostat_ctx->type;
  216		struct f2fs_sb_info *sbi = iostat_ctx->sbi;
  217		struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
  218		unsigned long flags;
  219
  220		if (!sbi->iostat_enable)
  221			return;
  222
  223		ts_diff = jiffies - iostat_ctx->submit_ts;
  224		if (page_type >= META_FLUSH)
                                 ^^^^^^^^^^

  225			page_type = META;
  226
  227		spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 @228		io_lat->sum_lat[type][page_type] += ts_diff;
                                      ^^^^^^^^^
Mixup between META_FLUSH and NR_PAGE_TYPE leads to memory corruption.

Fixes: a4b6817625e7 ("f2fs: introduce periodic iostat io latency traces")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/iostat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
index ed8176939aa5..e9a3df7ce4d9 100644
--- a/fs/f2fs/iostat.c
+++ b/fs/f2fs/iostat.c
@@ -223,7 +223,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 		return;
 
 	ts_diff = jiffies - iostat_ctx->submit_ts;
-	if (iotype >= META_FLUSH)
+	if (iotype == META_FLUSH)
 		iotype = META;
 
 	if (rw == 0) {
@@ -235,6 +235,11 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
 			idx = WRITE_ASYNC_IO;
 	}
 
+	if (iotype >= NR_PAGE_TYPE) {
+		f2fs_bug_on(sbi, 1);
+		return;
+	}
+
 	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
 	io_lat->sum_lat[idx][iotype] += ts_diff;
 	io_lat->bio_cnt[idx][iotype]++;
-- 
2.25.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

[Chao Yu]

On 2023/1/16 21:02, Yangtao Li wrote:
> Add iotype sanity check to avoid potential memory corruption.
> This is to fix the compile error below:
> 
> fs/f2fs/iostat.c:231 __update_iostat_latency() error: buffer overflow
> 'io_lat->peak_lat[type]' 3 <= 3
> 
> vim +228 fs/f2fs/iostat.c
> 
>    211  static inline void __update_iostat_latency(struct bio_iostat_ctx
> 	*iostat_ctx,
>    212					enum iostat_lat_type type)
>    213  {
>    214		unsigned long ts_diff;
>    215		unsigned int page_type = iostat_ctx->type;
>    216		struct f2fs_sb_info *sbi = iostat_ctx->sbi;
>    217		struct iostat_lat_info *io_lat = sbi->iostat_io_lat;
>    218		unsigned long flags;
>    219
>    220		if (!sbi->iostat_enable)
>    221			return;
>    222
>    223		ts_diff = jiffies - iostat_ctx->submit_ts;
>    224		if (page_type >= META_FLUSH)
>                                   ^^^^^^^^^^
> 
>    225			page_type = META;
>    226
>    227		spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   @228		io_lat->sum_lat[type][page_type] += ts_diff;
>                                        ^^^^^^^^^
> Mixup between META_FLUSH and NR_PAGE_TYPE leads to memory corruption.
> 
> Fixes: a4b6817625e7 ("f2fs: introduce periodic iostat io latency traces")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/iostat.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/iostat.c b/fs/f2fs/iostat.c
> index ed8176939aa5..e9a3df7ce4d9 100644
> --- a/fs/f2fs/iostat.c
> +++ b/fs/f2fs/iostat.c
> @@ -223,7 +223,7 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   		return;
>   
>   	ts_diff = jiffies - iostat_ctx->submit_ts;
> -	if (iotype >= META_FLUSH)
> +	if (iotype == META_FLUSH)

Maybe it's betterr to merge these two check condition as below?

if (iotype >= NR_PAGE_TYPE) {
	f2fs_bug_on(sbi, iotype != META_FLUSH);
	iotype = META;
}

For CHECK_FS is off case, I guess it's fine to not return and just print
warning message for notice.

Thanks,

>   		iotype = META;
>   
>   	if (rw == 0) {
> @@ -235,6 +235,11 @@ static inline void __update_iostat_latency(struct bio_iostat_ctx *iostat_ctx,
>   			idx = WRITE_ASYNC_IO;
>   	}
>   
> +	if (iotype >= NR_PAGE_TYPE) {
> +		f2fs_bug_on(sbi, 1);
> +		return;
> +	}
> +
>   	spin_lock_irqsave(&sbi->iostat_lat_lock, flags);
>   	io_lat->sum_lat[idx][iotype] += ts_diff;
>   	io_lat->bio_cnt[idx][iotype]++;


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

[Yangtao Li via Linux-f2fs-devel]

Hi Chao,

> Maybe it's betterr to merge these two check condition as below?
>
> if (iotype >= NR_PAGE_TYPE) {
> 	f2fs_bug_on(sbi, iotype != META_FLUSH);
> 	iotype = META;
> }

For normal , only META_FLUSH will be greater than NR_PAGE_TYPE,
there is no problem with this logic.

>
> For CHECK_FS is off case, I guess it's fine to not return and just print
> warning message for notice.

But if there is an exception, we don't know the type we originally wanted to count.
If they are all changed to meta, it is possible to get a wrong statistic. I think
there is no need to make statistics on this kind of error scene. Just like in the
next patch, if iostat_lat_type is wrong, we should return directly instead of changing
the value beyond the range to WRITE_ASYNC_IO.

So it's no need tp merge these two check condition?

Thx,
Yangtao


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 1/2] f2fs: fix to avoid potential memory corruption in __update_iostat_latency()

[Jaegeuk Kim]

On 01/16, Yangtao Li wrote:
> Hi Chao,
> 
> > Maybe it's betterr to merge these two check condition as below?
> >
> > if (iotype >= NR_PAGE_TYPE) {
> > 	f2fs_bug_on(sbi, iotype != META_FLUSH);
> > 	iotype = META;
> > }
> 
> For normal , only META_FLUSH will be greater than NR_PAGE_TYPE,
> there is no problem with this logic.
> 
> >
> > For CHECK_FS is off case, I guess it's fine to not return and just print
> > warning message for notice.
> 
> But if there is an exception, we don't know the type we originally wanted to count.
> If they are all changed to meta, it is possible to get a wrong statistic. I think
> there is no need to make statistics on this kind of error scene. Just like in the
> next patch, if iostat_lat_type is wrong, we should return directly instead of changing
> the value beyond the range to WRITE_ASYNC_IO.
> 
> So it's no need tp merge these two check condition?

I also prefer to do like Chao's comment. We don't need to expect such exception
at all.

And, it seems we just need to do like:

	enum page_type iotype;

	if (iotype == META_FLUSH) {
		iotype = META;
	} else if (iotype >= NR_PAGE_TYPE) {
		f2fs_warn();
		return;
	}

> 
> Thx,
> Yangtao


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel