This series tries to fix the potential cmem->ranges array overflow.
Yuntao Wang (2):
x86/crash: fix potential cmem->ranges array overflow
crash_core: fix out-of-bounds access check in
crash_exclude_mem_range()
arch/x86/kernel/crash.c | 9 +++++----
kernel/crash_core.c | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
--
2.43.0
The max_nr_ranges field of cmem allocated in crash_setup_memmap_entries()
is not initialized, its default value is 0.
When elfcorehdr is allocated from the middle of crashk_res due to any
potential reason, that is, `image->elf_load_addr > crashk_res.start &&
image->elf_load_addr + image->elf_headers_sz - 1 < crashk_res.end`,
executing memmap_exclude_ranges() will cause a range split to occur in
crash_exclude_mem_range(), which eventually leads to an overflow of the
cmem->ranges array.
Set cmem->max_nr_ranges to 1 to make crash_exclude_mem_range() return
-ENOMEM instead of causing cmem->ranges array overflow even when a split
happens.
Signed-off-by: Yuntao Wang <[email protected]>
---
arch/x86/kernel/crash.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..3be46f4b441e 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -282,10 +282,6 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
struct crash_memmap_data cmd;
struct crash_mem *cmem;
- cmem = vzalloc(struct_size(cmem, ranges, 1));
- if (!cmem)
- return -ENOMEM;
-
memset(&cmd, 0, sizeof(struct crash_memmap_data));
cmd.params = params;
@@ -321,6 +317,11 @@ int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
}
/* Exclude some ranges from crashk_res and add rest to memmap */
+ cmem = vzalloc(struct_size(cmem, ranges, 1));
+ if (!cmem)
+ return -ENOMEM;
+ cmem->max_nr_ranges = 1;
+
ret = memmap_exclude_ranges(image, cmem, crashk_res.start, crashk_res.end);
if (ret)
goto out;
--
2.43.0
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.
Signed-off-by: Yuntao Wang <[email protected]>
---
kernel/crash_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index d4313b53837e..991494d4cf43 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -627,7 +627,7 @@ int crash_exclude_mem_range(struct crash_mem *mem,
return 0;
/* Split happened */
- if (i == mem->max_nr_ranges - 1)
+ if (mem->nr_ranges >= mem->max_nr_ranges)
return -ENOMEM;
/* Location where new range should go */
--
2.43.0
Hi Yuntao,
On 12/18/23 at 04:19pm, Yuntao Wang wrote:
> This series tries to fix the potential cmem->ranges array overflow.
This series looks good to me. While you'd better talk to fuqiang to ask
if he wants to post these or wants to give up. He posted patch to raise
the potention issue and I suggested him to do these during the
discussion. Without consulting him for opinion to take over a discussing
work, it's not suggested, I would say.
https://lore.kernel.org/all/ZXrY7QbXAlxydsSC@MiWiFi-R3L-srv/T/#u
>
> Yuntao Wang (2):
> x86/crash: fix potential cmem->ranges array overflow
> crash_core: fix out-of-bounds access check in
> crash_exclude_mem_range()
>
> arch/x86/kernel/crash.c | 9 +++++----
> kernel/crash_core.c | 2 +-
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> --
> 2.43.0
>
On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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?
On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <[email protected]> wrote:
> On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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:
https://lore.kernel.org/lkml/[email protected]/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 Yuntao,
On 12/19/23 at 10:02am, Yuntao Wang wrote:
> On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <[email protected]> wrote:
>
> > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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/[email protected]/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
>
On Tue, 19 Dec 2023 11:32:02 +0800, Baoquan He <[email protected]> wrote:
> Hi Yuntao,
>
> On 12/19/23 at 10:02am, Yuntao Wang wrote:
> > On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <[email protected]> wrote:
> >
> > > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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/[email protected]/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
On 12/19/23 at 12:31pm, Yuntao Wang wrote:
> On Tue, 19 Dec 2023 11:32:02 +0800, Baoquan He <[email protected]> wrote:
> > Hi Yuntao,
> >
> > On 12/19/23 at 10:02am, Yuntao Wang wrote:
> > > On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <[email protected]> wrote:
> > >
> > > > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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/[email protected]/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.
In iomem, parent range includes children's range, while
walk_system_ram_res() traverses ranges not overlapped with each otehr.
From code in __walk_iomem_res_desc() and find_next_iomem_res(), it
clearly shows that.
walk_system_ram_res()
-->__walk_iomem_res_desc()
-->find_next_iomem_res()
>
> Finally, I would like to apologize again if my previous response offended you.
> That was not my intention.
No offence felt at all, and no worry about this. In upstream, argument
is normal, it's fine as long as your intention is making things better,
not against person. Meantime, let's be kind and friendly to each other,
we will have a great time.
On Tue, 19 Dec 2023 22:22:47 +0800, Baoquan He <[email protected]> wrote:
> On 12/19/23 at 12:31pm, Yuntao Wang wrote:
> > On Tue, 19 Dec 2023 11:32:02 +0800, Baoquan He <[email protected]> wrote:
> > > Hi Yuntao,
> > >
> > > On 12/19/23 at 10:02am, Yuntao Wang wrote:
> > > > On Mon, 18 Dec 2023 09:29:02 -0800, Andrew Morton <[email protected]> wrote:
> > > >
> > > > > On Mon, 18 Dec 2023 16:19:15 +0800 Yuntao Wang <[email protected]> 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/[email protected]/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.
>
> In iomem, parent range includes children's range, while
> walk_system_ram_res() traverses ranges not overlapped with each otehr.
> From code in __walk_iomem_res_desc() and find_next_iomem_res(), it
> clearly shows that.
>
> walk_system_ram_res()
> -->__walk_iomem_res_desc()
> -->find_next_iomem_res()
>
I revisited the relevant code, and yes, you are correct. The memory ranges
obtained from iomem do not overlap.
The reason why I thought these memory ranges would overlap was that I saw
that in the find_next_iomem_res() function, after traversing a parent node,
it starts to traverse its child nodes. If all these nodes meet our
requirements, then the memory ranges they represent will overlap.
However, I overlooked a very important point, which is that after finding a
valid node, the __walk_iomem_res_desc() function will update the start value.
This means that if a parent node is a valid node, all of its child nodes
will be skipped. This ultimately ensures that the memory ranges obtained
from iomem will not overlap.
I will post another patch later, optimizing crash_exclude_mem_range() using
your approach.
>
> >
> > Finally, I would like to apologize again if my previous response offended you.
> > That was not my intention.
>
> No offence felt at all, and no worry about this. In upstream, argument
> is normal, it's fine as long as your intention is making things better,
> not against person. Meantime, let's be kind and friendly to each other,
> we will have a great time.
Because memory ranges in mem->ranges are stored in ascending order, when we
detect `p_end < start`, we can break the for loop early, as the subsequent
memory ranges must also be outside the range we are looking for.
Signed-off-by: Yuntao Wang <[email protected]>
---
Hi Andrew,
Patch "[PATCH 2/2] crash_core: fix out-of-bounds access check in
crash_exclude_mem_range()" can be ignored, use this patch instead.
kernel/crash_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 9a219a918638..d425c4a106cd 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -575,9 +575,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;
--
2.43.0
On Wed, 20 Dec 2023 00:34:18 +0800 Yuntao Wang <[email protected]> wrote:
> Because memory ranges in mem->ranges are stored in ascending order, when we
> detect `p_end < start`, we can break the for loop early, as the subsequent
> memory ranges must also be outside the range we are looking for.
>
> Signed-off-by: Yuntao Wang <[email protected]>
> ---
> Hi Andrew,
>
> Patch "[PATCH 2/2] crash_core: fix out-of-bounds access check in
> crash_exclude_mem_range()" can be ignored, use this patch instead.
>
Some reviewer input on this would be helpful please?
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -575,9 +575,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;
> --
> 2.43.0
On 12/29/23 at 12:10pm, Andrew Morton wrote:
> On Wed, 20 Dec 2023 00:34:18 +0800 Yuntao Wang <[email protected]> wrote:
>
> > Because memory ranges in mem->ranges are stored in ascending order, when we
> > detect `p_end < start`, we can break the for loop early, as the subsequent
> > memory ranges must also be outside the range we are looking for.
> >
> > Signed-off-by: Yuntao Wang <[email protected]>
> > ---
> > Hi Andrew,
> >
> > Patch "[PATCH 2/2] crash_core: fix out-of-bounds access check in
> > crash_exclude_mem_range()" can be ignored, use this patch instead.
> >
>
> Some reviewer input on this would be helpful please?
I suggested this in below discussion thread:
https://lore.kernel.org/all/ZYEOshALGbDKwSdc@MiWiFi-R3L-srv/T/#u
So it would be good if squashing this into patch 3 of another patch
thread you are asking:
[PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()
And I would suggest withdrawing Yuntao's below patch on your
mm-nonmm-unstable branch.
961c69e9f1bf x86/crash: fix potential cmem->ranges array overflow
Becase there's better one to fix the potential oob from fuqiang,
although fuqiang need improve his patch log.
[PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds
https://lore.kernel.org/all/[email protected]/T/#u
>
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -575,9 +575,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;
> > --
> > 2.43.0
>
On Sat, 30 Dec 2023 18:28:06 +0800, Baoquan He <[email protected]> wrote:
> On 12/29/23 at 12:10pm, Andrew Morton wrote:
> > On Wed, 20 Dec 2023 00:34:18 +0800 Yuntao Wang <[email protected]> wrote:
> >
> > > Because memory ranges in mem->ranges are stored in ascending order, when we
> > > detect `p_end < start`, we can break the for loop early, as the subsequent
> > > memory ranges must also be outside the range we are looking for.
> > >
> > > Signed-off-by: Yuntao Wang <[email protected]>
> > > ---
> > > Hi Andrew,
> > >
> > > Patch "[PATCH 2/2] crash_core: fix out-of-bounds access check in
> > > crash_exclude_mem_range()" can be ignored, use this patch instead.
> > >
> >
> > Some reviewer input on this would be helpful please?
>
>
> I suggested this in below discussion thread:
> https://lore.kernel.org/all/ZYEOshALGbDKwSdc@MiWiFi-R3L-srv/T/#u
>
> So it would be good if squashing this into patch 3 of another patch
> thread you are asking:
> [PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()
>
Hi all,
I've squashed this patch into the patch:
[PATCH 3/3] crash_core: fix and simplify the logic of crash_exclude_mem_range()
The link to the new patch is:
https://lore.kernel.org/lkml/[email protected]/t/#m255d0d26148f2b384f6b7ab77eb38edf3f1bc0df
> And I would suggest withdrawing Yuntao's below patch on your
> mm-nonmm-unstable branch.
>
> 961c69e9f1bf x86/crash: fix potential cmem->ranges array overflow
>
> Becase there's better one to fix the potential oob from fuqiang,
> although fuqiang need improve his patch log.
>
> [PATCH v3] x86/kexec: fix potential cmem->ranges out of bounds
> https://lore.kernel.org/all/[email protected]/T/#u
>
I'm okay with that.
> >
> > > --- a/kernel/crash_core.c
> > > +++ b/kernel/crash_core.c
> > > @@ -575,9 +575,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;
> > > --
> > > 2.43.0
> >