Received-SPF: pass (google.com: domain of linux-kernel+bounces-55622-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240202135030.42265-1-csd4492@csd.uoc.gr> <CAHVXubioUY=vvQrLrcb+5aWav5WUdpJ3i0fCKP_xx0ASU30g=A@mail.gmail.com>
In-Reply-To: <CAHVXubioUY=vvQrLrcb+5aWav5WUdpJ3i0fCKP_xx0ASU30g=A@mail.gmail.com>
From: Alexandre Ghiti <alexghiti@rivosinc.com>
Date: Tue, 6 Feb 2024 22:17:44 +0100
Message-ID: <CAHVXubjanrxkDrNTZMrPtmewjqyf3L4_TTtqXhjGV+k+qkynmA@mail.gmail.com>
Subject: Re: [PATCH] [RFC] sparsemem: warn on out-of-bounds initialization
To: Dimitris Vlachos <csd4492@csd.uoc.gr>
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, 
	clameter@sgi.com, akpm@linux-foundation.org, rppt@kernel.org, arnd@arndb.de, 
	paul.walmsley@sifive.com, palmer@dabbelt.com, mick@ics.forth.gr
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Dimitris,

On Fri, Feb 2, 2024 at 2:59=E2=80=AFPM Alexandre Ghiti <alexghiti@rivosinc.=
com> wrote:
>
> Hi Dimitris,
>
> On Fri, Feb 2, 2024 at 2:50=E2=80=AFPM Dimitris Vlachos <csd4492@csd.uoc.=
gr> wrote:
> >
> > From: Dimitris Vlachos <dvlachos@ics.forth.gr>
> >
> > Hello all
> >
> > I am sending this email with regards to a bug that I discovered in the =
Sparse Memory Model configuration and more specifically, the Virtual Memory=
 Map optimization. Moreover, I would like to inquire about possible ways of=
 fixing it.
> >
> > I work as a pre-graduate research assistant at ICS-FORTH in the Compute=
r Architecture and VLSI Systems laboratory.
> > We were running some tests in our prototype hardware (RISC-V), where we=
 noticed that the Kernel crashes early in the boot process with the followi=
ng setup:
> >
> > We are using the default Kconfig configurations (defconfig) that includ=
es Sparse Memory + Virtual Memory Map.
> > The DRAM base address of our system is : 0x800000000000
> > A 3-level page table is used (Sv39).
> >
> > When the vmemmap optimization is enabled the macro pfn_to_page() is cal=
led, which offsets the vmemmap with the pfn argument to acquire a struct pa=
ge pointer.
> >
> > As our DRAM starts at 0x800000000000, the initial pfn will be 0x8000000=
00000 divided by PAGE_SIZE. The calculation result will be:
> > 0xffffffcf00000000 (vmemmap start) +  (0x800000000 (pfn) * 64 (sizeof(s=
truct page))
> >
> > This causes an overflow as the number is very large, the resulting addr=
ess is 0x1c700000000 which is not a valid Sv39 address (all bits above bit =
38 should be set) and does not belong to the kernel=E2=80=99s virtual addre=
ss space.
> >
> > The same will happen with all valid pfns as the memory is being initial=
ized. When the Kernel will try to access a page using pfn_to_page, a page f=
ault will occur crashing the system.
> > It should be noted that when the vmemmap is disabled the system boots n=
ormally.
> >
> > However, I would like to emphasize another important detail. When the D=
RAM base is small enough to avoid an overflow, the virtual memory map mappi=
ngs will be initialized out of bounds with regard to the vmemmap address sp=
ace which is 4GiB in Sv39.
> > The system will not crash, but the address space used for this purpose =
will be outside of the designated range.
> >
> > Everything mentioned above holds true when Sv48 is used as well. Given =
a high enough DRAM base the system will either crash or perform false initi=
alizations. Given the fact that virtual memory map is not only used by RISC=
-V systems, this could be an issue for other architectures as well.
> >
> > The kernel already contains mminit_validate_memmodel_limits() that chec=
ks memory limits.
> > However, it only checks physical memory ranges. I added a few checks, p=
rovided that virtual memory map is enabled, to determine whether offset sta=
rt and offset end fall inside the range of virtual memory map. Otherwise, a=
 warning will be printed.
> >
> > Finally, I would like to ask you for any information/advice that could =
help me fix/prevent the issues that I mentioned (if it=E2=80=99s possible) =
or recover from them at runtime by disabling the optimization and reverting=
 back to the non-vmemmap mappings.
> >
> > Thank you in advance.
> > Best regards,
> > Dimitris Vlachos
> > ---
> >  mm/sparse.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 338cf946d..33b57758e 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -149,6 +149,26 @@ static void __meminit mminit_validate_memmodel_lim=
its(unsigned long *start_pfn,
> >                 WARN_ON_ONCE(1);
> >                 *end_pfn =3D max_sparsemem_pfn;
> >         }
> > +
> > +       /*check vmemmap limits*/
> > +       #ifdef CONFIG_SPARSEMEM_VMEMMAP
> > +
> > +       unsigned long vmemmap_offset_start =3D (unsigned long) pfn_to_p=
age(*start_pfn);
> > +       unsigned long vmemmap_offset_end   =3D (unsigned long) pfn_to_p=
age(*end_pfn);
> > +
> > +       if (vmemmap_offset_start < VMEMMAP_START) {
> > +               mminit_dprintk(MMINIT_WARNING, "pfnvalidation",
> > +                       "Start of range %lu -> %lu exceeds bounds of SP=
ARSEMEM virtual memory map address range %lu -> %lu\n",
> > +                       vmemmap_offset_start, vmemmap_offset_end, VMEMM=
AP_START,VMEMMAP_END);
> > +               WARN_ON_ONCE(1);
> > +
> > +       } else if (vmemmap_offset_end > VMEMMAP_END ) {
> > +               mminit_dprintk(MMINIT_WARNING, "pfnvalidation",
> > +                       "End of range %lu -> %lu exceeds bounds of SPAR=
SEMEM virtual memory map address range %lu -> %lu\n",
> > +                       vmemmap_offset_start, vmemmap_offset_end, VMEMM=
AP_START,VMEMMAP_END);
> > +               WARN_ON_ONCE(1);
> > +       }
> > +       #endif
> >  }
> >
> >  /*
> > --
> > 2.39.2
> >
>
> Since struct pages are only available for memory,  we could offset
> vmemmap so that VMEMMAP_START actually points to the first pfn?

> Hello Alexandre,
> My first thought was to use a "better" offset for vmemmap as well.Since p=
fn is essential for pfn_to_page and page_to_pfn i think that maybe we could=
 get a smaller number out of pfn to offset the vmemmap and ensure that it's=
  bounds are respected for pfn_to_page operation.
> If we use only a part of the pfn for the offset we could get the  pfn fro=
m page_to_pfn by adding PAGE-VMEMMAP_START
> to the DRAM base/PAGE_SIZE which is the first pfn.

Let's keep the discussion here.

IIUC you propose to change pfn_to_page()/page_to_pfn() but what I
proposed was to directly offset vmemmap like this:

#define vmemmap         ((struct page *)VMEMMAP_START - phys_ram_base
>> PAGE_SHIFT)

So that the first page in vmemmap corresponds to the first page of
physical memory, that avoids the burden of changing the conversion
macros, WDYT?

Thanks,

Alex