2023-06-17 05:09:25

by Liam Ni

Subject: [PATCH] mm/sparse:avoid null pointer access in memory_present()

__nr_to_section() may return a null pointer,
before accessing the member variable section_mem_map,
we should first determine whether it is a null pointer.

Signed-off-by: Liam Ni <[email protected]>
---
mm/sparse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 4e6e3a9d49dc..37fa3818bc25 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
set_section_nid(section, nid);

ms = __nr_to_section(section);
- if (!ms->section_mem_map) {
+ if (ms && !ms->section_mem_map) {
ms->section_mem_map = sparse_encode_early_nid(nid) |
SECTION_IS_ONLINE;
__section_mark_present(ms, section);
--
2.25.1

2023-06-17 06:15:36

by Andrew Morton

Subject: Re: [PATCH] mm/sparse:avoid null pointer access in memory_present()

On Sat, 17 Jun 2023 14:40:36 +1000 Liam Ni <[email protected]> wrote:

> __nr_to_section() may return a null pointer,
> before accessing the member variable section_mem_map,
> we should first determine whether it is a null pointer.
>
> ...
>
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> set_section_nid(section, nid);
>
> ms = __nr_to_section(section);
> - if (!ms->section_mem_map) {
> + if (ms && !ms->section_mem_map) {
> ms->section_mem_map = sparse_encode_early_nid(nid) |
> SECTION_IS_ONLINE;
> __section_mark_present(ms, section);

I'm suspecting that if __nr_to_section() returns NULL here, we should
just panic. But a null-deref gives the same information, so why change
things?

2023-06-17 06:39:54

by Liam Ni

Subject: Re: [PATCH] mm/sparse:avoid null pointer access in memory_present()

On Sat, 17 Jun 2023 at 13:44, Andrew Morton <[email protected]> wrote:
>
> On Sat, 17 Jun 2023 14:40:36 +1000 Liam Ni <[email protected]> wrote:
>
> > __nr_to_section() may return a null pointer,
> > before accessing the member variable section_mem_map,
> > we should first determine whether it is a null pointer.
> >
> > ...
> >
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > set_section_nid(section, nid);
> >
> > ms = __nr_to_section(section);
> > - if (!ms->section_mem_map) {
> > + if (ms && !ms->section_mem_map) {
> > ms->section_mem_map = sparse_encode_early_nid(nid) |
> > SECTION_IS_ONLINE;
> > __section_mark_present(ms, section);
>
> I'm suspecting that if __nr_to_section() returns NULL here, we should
> just panic. But a null-deref gives the same information, so why change
> things?

Do you mean if ms is a null pointer，ms->section_mem_map will cause
system panic,so we needn't change?

>

2023-06-17 07:12:50

by Mike Rapoport

Subject: Re: [PATCH] mm/sparse:avoid null pointer access in memory_present()

On Sat, Jun 17, 2023 at 02:17:58PM +0800, Liam Ni wrote:
> On Sat, 17 Jun 2023 at 13:44, Andrew Morton <[email protected]> wrote:
> >
> > On Sat, 17 Jun 2023 14:40:36 +1000 Liam Ni <[email protected]> wrote:
> >
> > > __nr_to_section() may return a null pointer,
> > > before accessing the member variable section_mem_map,
> > > we should first determine whether it is a null pointer.
> > >
> > > ...
> > >
> > > --- a/mm/sparse.c
> > > +++ b/mm/sparse.c
> > > @@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > > set_section_nid(section, nid);
> > >
> > > ms = __nr_to_section(section);
> > > - if (!ms->section_mem_map) {
> > > + if (ms && !ms->section_mem_map) {
> > > ms->section_mem_map = sparse_encode_early_nid(nid) |
> > > SECTION_IS_ONLINE;
> > > __section_mark_present(ms, section);
> >
> > I'm suspecting that if __nr_to_section() returns NULL here, we should
> > just panic. But a null-deref gives the same information, so why change
> > things?
>
> Do you mean if ms is a null pointer，ms->section_mem_map will cause
> system panic,so we needn't change?

Yes, if __nr_to_section ever returns NULL the system will crash anyway.

--
Sincerely yours,
Mike.

2023-06-17 09:50:30

by Liam Ni

Subject: Re: [PATCH] mm/sparse:avoid null pointer access in memory_present()

On Sat, 17 Jun 2023 at 15:01, Mike Rapoport <[email protected]> wrote:
>
> On Sat, Jun 17, 2023 at 02:17:58PM +0800, Liam Ni wrote:
> > On Sat, 17 Jun 2023 at 13:44, Andrew Morton <[email protected]> wrote:
> > >
> > > On Sat, 17 Jun 2023 14:40:36 +1000 Liam Ni <[email protected]> wrote:
> > >
> > > > __nr_to_section() may return a null pointer,
> > > > before accessing the member variable section_mem_map,
> > > > we should first determine whether it is a null pointer.
> > > >
> > > > ...
> > > >
> > > > --- a/mm/sparse.c
> > > > +++ b/mm/sparse.c
> > > > @@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > > > set_section_nid(section, nid);
> > > >
> > > > ms = __nr_to_section(section);
> > > > - if (!ms->section_mem_map) {
> > > > + if (ms && !ms->section_mem_map) {
> > > > ms->section_mem_map = sparse_encode_early_nid(nid) |
> > > > SECTION_IS_ONLINE;
> > > > __section_mark_present(ms, section);
> > >
> > > I'm suspecting that if __nr_to_section() returns NULL here, we should
> > > just panic. But a null-deref gives the same information, so why change
> > > things?
> >
> > Do you mean if ms is a null pointer，ms->section_mem_map will cause
> > system panic,so we needn't change?
>
> Yes, if __nr_to_section ever returns NULL the system will crash anyway.

I got it,do we need to print some information by panic()?

>
> --
> Sincerely yours,
> Mike.

2023-06-18 07:50:50

by Mike Rapoport

Subject: Re: [PATCH] mm/sparse:avoid null pointer access in memory_present()

On Sat, Jun 17, 2023 at 04:59:46PM +0800, Liam Ni wrote:
> On Sat, 17 Jun 2023 at 15:01, Mike Rapoport <[email protected]> wrote:
> >
> > On Sat, Jun 17, 2023 at 02:17:58PM +0800, Liam Ni wrote:
> > > On Sat, 17 Jun 2023 at 13:44, Andrew Morton <[email protected]> wrote:
> > > >
> > > > On Sat, 17 Jun 2023 14:40:36 +1000 Liam Ni <[email protected]> wrote:
> > > >
> > > > > __nr_to_section() may return a null pointer,
> > > > > before accessing the member variable section_mem_map,
> > > > > we should first determine whether it is a null pointer.
> > > > >
> > > > > ...
> > > > >
> > > > > --- a/mm/sparse.c
> > > > > +++ b/mm/sparse.c
> > > > > @@ -258,7 +258,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > > > > set_section_nid(section, nid);
> > > > >
> > > > > ms = __nr_to_section(section);
> > > > > - if (!ms->section_mem_map) {
> > > > > + if (ms && !ms->section_mem_map) {
> > > > > ms->section_mem_map = sparse_encode_early_nid(nid) |
> > > > > SECTION_IS_ONLINE;
> > > > > __section_mark_present(ms, section);
> > > >
> > > > I'm suspecting that if __nr_to_section() returns NULL here, we should
> > > > just panic. But a null-deref gives the same information, so why change
> > > > things?
> > >
> > > Do you mean if ms is a null pointer，ms->section_mem_map will cause
> > > system panic,so we needn't change?
> >
> > Yes, if __nr_to_section ever returns NULL the system will crash anyway.
>
> I got it,do we need to print some information by panic()?

Accessing a NULL pointer will cause panic and there will be lots of
information spilled into the log anyway.

--
Sincerely yours,
Mike.