Received-SPF: pass (google.com: domain of linux-kernel+bounces-49923-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>
In-Reply-To: <20240202135030.42265-1-csd4492@csd.uoc.gr>
From: Alexandre Ghiti <alexghiti@rivosinc.com>
Date: Fri, 2 Feb 2024 14:59:30 +0100
Message-ID: <CAHVXubioUY=vvQrLrcb+5aWav5WUdpJ3i0fCKP_xx0ASU30g=A@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: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 Sp=
arse Memory Model configuration and more specifically, the Virtual Memory M=
ap optimization. Moreover, I would like to inquire about possible ways of f=
ixing it.
>
> I work as a pre-graduate research assistant at ICS-FORTH in the Computer =
Architecture and VLSI Systems laboratory.
> We were running some tests in our prototype hardware (RISC-V), where we n=
oticed that the Kernel crashes early in the boot process with the following=
 setup:
>
> We are using the default Kconfig configurations (defconfig) that includes=
 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 calle=
d, which offsets the vmemmap with the pfn argument to acquire a struct page=
 pointer.
>
> As our DRAM starts at 0x800000000000, the initial pfn will be 0x800000000=
000 divided by PAGE_SIZE. The calculation result will be:
> 0xffffffcf00000000 (vmemmap start) +  (0x800000000 (pfn) * 64 (sizeof(str=
uct page))
>
> This causes an overflow as the number is very large, the resulting addres=
s 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 address=
 space.
>
> The same will happen with all valid pfns as the memory is being initializ=
ed. When the Kernel will try to access a page using pfn_to_page, a page fau=
lt will occur crashing the system.
> It should be noted that when the vmemmap is disabled the system boots nor=
mally.
>
> However, I would like to emphasize another important detail. When the DRA=
M base is small enough to avoid an overflow, the virtual memory map mapping=
s will be initialized out of bounds with regard to the vmemmap address spac=
e which is 4GiB in Sv39.
> The system will not crash, but the address space used for this purpose wi=
ll 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 initial=
izations. 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 checks=
 memory limits.
> However, it only checks physical memory ranges. I added a few checks, pro=
vided that virtual memory map is enabled, to determine whether offset start=
 and offset end fall inside the range of virtual memory map. Otherwise, a w=
arning will be printed.
>
> Finally, I would like to ask you for any information/advice that could he=
lp 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 b=
ack 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_limit=
s(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_pag=
e(*start_pfn);
> +       unsigned long vmemmap_offset_end   =3D (unsigned long) pfn_to_pag=
e(*end_pfn);
> +
> +       if (vmemmap_offset_start < VMEMMAP_START) {
> +               mminit_dprintk(MMINIT_WARNING, "pfnvalidation",
> +                       "Start of range %lu -> %lu exceeds bounds of SPAR=
SEMEM virtual memory map address range %lu -> %lu\n",
> +                       vmemmap_offset_start, vmemmap_offset_end, VMEMMAP=
_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 SPARSE=
MEM virtual memory map address range %lu -> %lu\n",
> +                       vmemmap_offset_start, vmemmap_offset_end, VMEMMAP=
_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?