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

[Baoquan He <bhe@redhat.com>]

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
>