Re: [PATCH 2/2] crash_core: fix out-of-bounds access check in crash_exclude_mem_range()

[Yuntao Wang <ytcoode@gmail.com>]

On Tue, 19 Dec 2023 11:32:02 +0800, Baoquan He <bhe@redhat.com> wrote:
> Hi Yuntao,
> 
> On 12/19/23 at 10:02am, Yuntao Wang wrote:
> > On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <ytcoode@gmail.com> wrote:
> > > 
> > > > mem->nr_ranges represents the current number of elements stored in
> > > > the mem->ranges array, and mem->max_nr_ranges represents the maximum number
> > > > of elements that the mem->ranges array can hold. Therefore, the correct
> > > > array out-of-bounds check should be mem->nr_ranges >= mem->max_nr_ranges.
> > > > 
> > > 
> > > This does not apply after your own "crash_core: fix and simplify the
> > > logic of crash_exclude_mem_range()".  What should be done?
> > 
> > Hi Andrew,
> > 
> > I actually prefer the "crash_core: fix and simplify the logic of
> > crash_exclude_mem_range()" patch as it makes the final code more concise and
> > clear, and less prone to errors.
> > 
> > The current code is too strange, I guess no one can understand why there is
> > a break in the for loop when they read this code for the first time.
> > 
> > Moreover, I think the current code is too fragile, it relies on callers using
> > this function correctly to ensure its correctness, rather than being able to
> > guarantee the correctness on its own. I even feel that this function is very
> > likely to have bugs again as the code evolves.
> > 
> > However, Baoquan also has his own considerations, he suggests keeping the code
> > as it is.
> > 
> > The link below is our detailed discussion on this issue:
> 
> There's misunderstanding here.
> 
> Firstly I said I have concern about the patch, I didn't NACK or reject the patch.
> 
> [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()
> 
> Usually, when people said he/she had concern, you may need to
> investigate and resolve it or explain why it's not need be cared about.
> 
> E.g on above [PATCH 3/3], we can add below code change to stop scanning
> when the left ranges are all above the excluded range, assume the passed
> in cmem has a ascending order of ranges. Say so because I checked code
> and found that crash_exclude_mem_range() is called in arch arm64, ppc,
> riscv and x86. Among them, arm64 and ppc create the cmem from memblock,
> riscv and x86 create cmem from iomem. All of them should be in ascending
> ordr. The below code change based on your patch 3/3 looks safe to me.
> What do you think?
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index aab342c2a5ee..39b6c149dc80 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -574,9 +574,12 @@ int crash_exclude_mem_range(struct crash_mem *mem,
>  		p_start = mstart;
>  		p_end = mend;
>  
> -		if (p_start > end || p_end < start)
> +		if (p_start > end)
>  			continue;
>  
> +		if (p_end < start)
> +			break;
> +
>  		/* Truncate any area outside of range */
>  		if (p_start < start)
>  			p_start = start;
> 
> Secondly, I welcome people who are interested kexec/kdump code, and raise
> issues or post patches to fix bug, clean up code. I like these patches.
> They can help improve kexec/kdump code and solve problem in advance.
> I would like to review and make the patches acceptable and merged
> inally. And I also hope people can follow the later issue reported by
> other people or LKP if their merged patch caused that.
> 
> Lastly, people are encouraged to help review other people's patch
> and give suggestes to improve the code change. If patch author don't
> respond for a long while or the work has been suspended for long time, we
> can add comment to tell and take over the work to continue.
> 
> These are my personal understanding and thought about kexec/kdump patch
> reviewing and maintance. So cheer up.
> 
> > 
> > https://lore.kernel.org/lkml/20231214163842.129139-3-ytcoode@gmail.com/t/#mfd78a97e16251bcb190b0957a0b6cb4b0a096b54
> > 
> > The final decision on whether to apply that patch is up to you and Baoquan, if
> > you choose to apply that patch, this patch can be ignored. But if you decide not
> > to apply that patch, then this patch must be applied, as it fixes a bug in the
> > crash_exclude_mem_range() function.
> > 
> > Sincerely,
> > Yuntao

Hi Baoquan,

I must clarify that I was not complaining about you. On the contrary, I am
grateful to everyone who takes time to review code for others, because I know
it is a lot of work.

I'm relatively new to the Linux community and still learning the various rules
of the community. I'm very sorry that I didn't fully grasp your previous intention.

Regarding the method you suggested to add a 'break', I did consider it initially
but later decided against it because the memory ranges obtained from iomem may
overlap, so I chose a safer way instead.

Finally, I would like to apologize again if my previous response offended you.
That was not my intention.

Sincerely,
Yuntao