* [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
@ 2019-08-19 5:14 Anup Patel
2019-08-26 6:44 ` Christoph Hellwig
2019-08-27 0:13 ` Paul Walmsley
0 siblings, 2 replies; 6+ messages in thread
From: Anup Patel @ 2019-08-19 5:14 UTC (permalink / raw)
To: Palmer Dabbelt, Paul Walmsley
Cc: Anup Patel, Anup Patel, linux-kernel, Christoph Hellwig,
Atish Patra, Alistair Francis, linux-riscv
Currently, various virtual memory areas of Linux RISC-V are organized
in increasing order of their virtual addresses is as follows:
1. User space area (This is lowest area and starts at 0x0)
2. FIXMAP area
3. VMALLOC area
4. Kernel area (This is highest area and starts at PAGE_OFFSET)
The maximum size of user space aread is represented by TASK_SIZE.
On RV32 systems, TASK_SIZE is defined as VMALLOC_START which causes the
user space area to overlap the FIXMAP area. This allows user space apps
to potentially corrupt the FIXMAP area and kernel OF APIs will crash
whenever they access corrupted FDT in the FIXMAP area.
On RV64 systems, TASK_SIZE is set to fixed 256GB and no other areas
happen to overlap so we don't see any FIXMAP area corruptions.
This patch fixes FIXMAP area corruption on RV32 systems by setting
TASK_SIZE to FIXADDR_START. We also move FIXADDR_TOP, FIXADDR_SIZE,
and FIXADDR_START defines to asm/pgtable.h so that we can avoid cyclic
header includes.
Signed-off-by: Anup Patel <anup.patel@wdc.com>
Tested-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
Changes since v1:
- Drop braces from "#define FIXADDR_TOP"
---
arch/riscv/include/asm/fixmap.h | 4 ----
arch/riscv/include/asm/pgtable.h | 12 ++++++++++--
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
index 9c66033c3a54..161f28d04a07 100644
--- a/arch/riscv/include/asm/fixmap.h
+++ b/arch/riscv/include/asm/fixmap.h
@@ -30,10 +30,6 @@ enum fixed_addresses {
__end_of_fixed_addresses
};
-#define FIXADDR_SIZE (__end_of_fixed_addresses * PAGE_SIZE)
-#define FIXADDR_TOP (VMALLOC_START)
-#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
-
#define FIXMAP_PAGE_IO PAGE_KERNEL
#define __early_set_fixmap __set_fixmap
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index a364aba23d55..c24a083b3e12 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -420,14 +420,22 @@ static inline void pgtable_cache_init(void)
#define VMALLOC_END (PAGE_OFFSET - 1)
#define VMALLOC_START (PAGE_OFFSET - VMALLOC_SIZE)
+#define FIXADDR_TOP VMALLOC_START
+#ifdef CONFIG_64BIT
+#define FIXADDR_SIZE PMD_SIZE
+#else
+#define FIXADDR_SIZE PGDIR_SIZE
+#endif
+#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
+
/*
- * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32.
+ * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
* Note that PGDIR_SIZE must evenly divide TASK_SIZE.
*/
#ifdef CONFIG_64BIT
#define TASK_SIZE (PGDIR_SIZE * PTRS_PER_PGD / 2)
#else
-#define TASK_SIZE VMALLOC_START
+#define TASK_SIZE FIXADDR_START
#endif
#include <asm-generic/pgtable.h>
--
2.17.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
2019-08-19 5:14 [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems Anup Patel
@ 2019-08-26 6:44 ` Christoph Hellwig
2019-08-27 0:13 ` Paul Walmsley
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2019-08-26 6:44 UTC (permalink / raw)
To: Anup Patel
Cc: Anup Patel, Palmer Dabbelt, linux-kernel, Christoph Hellwig,
Atish Patra, Alistair Francis, Paul Walmsley, linux-riscv
Palmer, Paul - are you going to pick this up? Seems like we've just
missed -rc6.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
2019-08-19 5:14 [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems Anup Patel
2019-08-26 6:44 ` Christoph Hellwig
@ 2019-08-27 0:13 ` Paul Walmsley
2019-08-27 2:41 ` Anup Patel
1 sibling, 1 reply; 6+ messages in thread
From: Paul Walmsley @ 2019-08-27 0:13 UTC (permalink / raw)
To: Anup Patel
Cc: Anup Patel, Palmer Dabbelt, linux-kernel, Christoph Hellwig,
Atish Patra, Alistair Francis, linux-riscv
Hello Anup,
On Mon, 19 Aug 2019, Anup Patel wrote:
> Currently, various virtual memory areas of Linux RISC-V are organized
> in increasing order of their virtual addresses is as follows:
> 1. User space area (This is lowest area and starts at 0x0)
> 2. FIXMAP area
> 3. VMALLOC area
> 4. Kernel area (This is highest area and starts at PAGE_OFFSET)
>
> The maximum size of user space aread is represented by TASK_SIZE.
>
> On RV32 systems, TASK_SIZE is defined as VMALLOC_START which causes the
> user space area to overlap the FIXMAP area. This allows user space apps
> to potentially corrupt the FIXMAP area and kernel OF APIs will crash
> whenever they access corrupted FDT in the FIXMAP area.
>
> On RV64 systems, TASK_SIZE is set to fixed 256GB and no other areas
> happen to overlap so we don't see any FIXMAP area corruptions.
>
> This patch fixes FIXMAP area corruption on RV32 systems by setting
> TASK_SIZE to FIXADDR_START.
This part -- the TASK_SIZE change -- makes sense to me.
However, the patch also changes FIXADDR_SIZE to be defined in terms of
page table-related constants. Previously, FIXADDR_SIZE was based on
__end_of_fixed_addresses, as it is for most other architectures. The part
of the patch that changes FIXADDR_SIZE seems unrelated to the actual fix.
If that's indeed the case -- that the change to FIXADDR_SIZE is unrelated
from the fix -- could you please split that into a separate patch, with a
description of the rationale? I think I understand why you're proposing
it, but it seems odd to explicitly connect it to page table-related
constants, rather than the contents of "enum fixed_addresses", and I'm
reluctant to merge that part of this patch without a bit more discussion.
> We also move FIXADDR_TOP, FIXADDR_SIZE, and FIXADDR_START defines to
> asm/pgtable.h so that we can avoid cyclic header includes.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Tested-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> Changes since v1:
> - Drop braces from "#define FIXADDR_TOP"
> ---
> arch/riscv/include/asm/fixmap.h | 4 ----
> arch/riscv/include/asm/pgtable.h | 12 ++++++++++--
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
> index 9c66033c3a54..161f28d04a07 100644
> --- a/arch/riscv/include/asm/fixmap.h
> +++ b/arch/riscv/include/asm/fixmap.h
> @@ -30,10 +30,6 @@ enum fixed_addresses {
> __end_of_fixed_addresses
> };
>
> -#define FIXADDR_SIZE (__end_of_fixed_addresses * PAGE_SIZE)
> -#define FIXADDR_TOP (VMALLOC_START)
> -#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
> -
> #define FIXMAP_PAGE_IO PAGE_KERNEL
>
> #define __early_set_fixmap __set_fixmap
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index a364aba23d55..c24a083b3e12 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -420,14 +420,22 @@ static inline void pgtable_cache_init(void)
> #define VMALLOC_END (PAGE_OFFSET - 1)
> #define VMALLOC_START (PAGE_OFFSET - VMALLOC_SIZE)
>
> +#define FIXADDR_TOP VMALLOC_START
> +#ifdef CONFIG_64BIT
> +#define FIXADDR_SIZE PMD_SIZE
> +#else
> +#define FIXADDR_SIZE PGDIR_SIZE
> +#endif
> +#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
> +
> /*
> - * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32.
> + * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
> * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
> */
> #ifdef CONFIG_64BIT
> #define TASK_SIZE (PGDIR_SIZE * PTRS_PER_PGD / 2)
> #else
> -#define TASK_SIZE VMALLOC_START
> +#define TASK_SIZE FIXADDR_START
> #endif
>
> #include <asm-generic/pgtable.h>
> --
> 2.17.1
>
- Paul
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
2019-08-27 0:13 ` Paul Walmsley
@ 2019-08-27 2:41 ` Anup Patel
2019-08-28 22:32 ` Alistair Francis
2019-08-28 22:33 ` Paul Walmsley
0 siblings, 2 replies; 6+ messages in thread
From: Anup Patel @ 2019-08-27 2:41 UTC (permalink / raw)
To: Paul Walmsley
Cc: Palmer Dabbelt, Anup Patel, linux-kernel, Christoph Hellwig,
Atish Patra, Alistair Francis, linux-riscv
On Tue, Aug 27, 2019 at 5:43 AM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> Hello Anup,
>
> On Mon, 19 Aug 2019, Anup Patel wrote:
>
> > Currently, various virtual memory areas of Linux RISC-V are organized
> > in increasing order of their virtual addresses is as follows:
> > 1. User space area (This is lowest area and starts at 0x0)
> > 2. FIXMAP area
> > 3. VMALLOC area
> > 4. Kernel area (This is highest area and starts at PAGE_OFFSET)
> >
> > The maximum size of user space aread is represented by TASK_SIZE.
> >
> > On RV32 systems, TASK_SIZE is defined as VMALLOC_START which causes the
> > user space area to overlap the FIXMAP area. This allows user space apps
> > to potentially corrupt the FIXMAP area and kernel OF APIs will crash
> > whenever they access corrupted FDT in the FIXMAP area.
> >
> > On RV64 systems, TASK_SIZE is set to fixed 256GB and no other areas
> > happen to overlap so we don't see any FIXMAP area corruptions.
> >
> > This patch fixes FIXMAP area corruption on RV32 systems by setting
> > TASK_SIZE to FIXADDR_START.
>
> This part -- the TASK_SIZE change -- makes sense to me.
>
> However, the patch also changes FIXADDR_SIZE to be defined in terms of
> page table-related constants. Previously, FIXADDR_SIZE was based on
> __end_of_fixed_addresses, as it is for most other architectures. The part
> of the patch that changes FIXADDR_SIZE seems unrelated to the actual fix.
>
> If that's indeed the case -- that the change to FIXADDR_SIZE is unrelated
> from the fix -- could you please split that into a separate patch, with a
> description of the rationale? I think I understand why you're proposing
> it, but it seems odd to explicitly connect it to page table-related
> constants, rather than the contents of "enum fixed_addresses", and I'm
> reluctant to merge that part of this patch without a bit more discussion.
The FIXADDR_SIZE change is related to the TASK_SIZE requirement and
it is not a separate change because:
1. TASK_SIZE must be evenly divisible by PGDIR_SIZE. The FIXADDR_START
is defined as (FIXADDR_TOP - FIXADDR_SIZE). The original FIXADDR_SIZE
defined in-terms of __end_of_fixed_addresses is not a multiple of PGDIR_SIZE
hence it makes sense to make FIXADDR_SIZE as PGDIR_SIZE.
2. Let say we ignore point1 above then still we cannot continue to express
FIXADDR_SIZE in-terms of __end_of_fixed_addresses because of cyclic
header dependency where asm/fixmap.h includes asm/pgtable.h and
__end_of_fixed_addresses is defined in asm/fixmap.h. We certainly need
to move FIXADDR_TOP, FIXADDR_START, and FIXADDR_SIZE to
asm/pgtable.h so that we can express TASK_SIZE as FIXADDR_START
for RV32. If we don't simplify FIXADDR_SIZE then it will result in compile
errors.
Regards,
Anup
>
>
> > We also move FIXADDR_TOP, FIXADDR_SIZE, and FIXADDR_START defines to
> > asm/pgtable.h so that we can avoid cyclic header includes.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > Tested-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > Changes since v1:
> > - Drop braces from "#define FIXADDR_TOP"
> > ---
> > arch/riscv/include/asm/fixmap.h | 4 ----
> > arch/riscv/include/asm/pgtable.h | 12 ++++++++++--
> > 2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/fixmap.h b/arch/riscv/include/asm/fixmap.h
> > index 9c66033c3a54..161f28d04a07 100644
> > --- a/arch/riscv/include/asm/fixmap.h
> > +++ b/arch/riscv/include/asm/fixmap.h
> > @@ -30,10 +30,6 @@ enum fixed_addresses {
> > __end_of_fixed_addresses
> > };
> >
> > -#define FIXADDR_SIZE (__end_of_fixed_addresses * PAGE_SIZE)
> > -#define FIXADDR_TOP (VMALLOC_START)
> > -#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
> > -
> > #define FIXMAP_PAGE_IO PAGE_KERNEL
> >
> > #define __early_set_fixmap __set_fixmap
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index a364aba23d55..c24a083b3e12 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -420,14 +420,22 @@ static inline void pgtable_cache_init(void)
> > #define VMALLOC_END (PAGE_OFFSET - 1)
> > #define VMALLOC_START (PAGE_OFFSET - VMALLOC_SIZE)
> >
> > +#define FIXADDR_TOP VMALLOC_START
> > +#ifdef CONFIG_64BIT
> > +#define FIXADDR_SIZE PMD_SIZE
> > +#else
> > +#define FIXADDR_SIZE PGDIR_SIZE
> > +#endif
> > +#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
> > +
> > /*
> > - * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32.
> > + * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
> > * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
> > */
> > #ifdef CONFIG_64BIT
> > #define TASK_SIZE (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > #else
> > -#define TASK_SIZE VMALLOC_START
> > +#define TASK_SIZE FIXADDR_START
> > #endif
> >
> > #include <asm-generic/pgtable.h>
> > --
> > 2.17.1
> >
>
>
> - Paul
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
2019-08-27 2:41 ` Anup Patel
@ 2019-08-28 22:32 ` Alistair Francis
2019-08-28 22:33 ` Paul Walmsley
1 sibling, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2019-08-28 22:32 UTC (permalink / raw)
To: anup, paul.walmsley
Cc: palmer, Anup Patel, linux-kernel, hch, Atish Patra, linux-riscv
On Tue, 2019-08-27 at 08:11 +0530, Anup Patel wrote:
> On Tue, Aug 27, 2019 at 5:43 AM Paul Walmsley <
> paul.walmsley@sifive.com> wrote:
> > Hello Anup,
> >
> > On Mon, 19 Aug 2019, Anup Patel wrote:
> >
> > > Currently, various virtual memory areas of Linux RISC-V are
> > > organized
> > > in increasing order of their virtual addresses is as follows:
> > > 1. User space area (This is lowest area and starts at 0x0)
> > > 2. FIXMAP area
> > > 3. VMALLOC area
> > > 4. Kernel area (This is highest area and starts at PAGE_OFFSET)
> > >
> > > The maximum size of user space aread is represented by TASK_SIZE.
> > >
> > > On RV32 systems, TASK_SIZE is defined as VMALLOC_START which
> > > causes the
> > > user space area to overlap the FIXMAP area. This allows user
> > > space apps
> > > to potentially corrupt the FIXMAP area and kernel OF APIs will
> > > crash
> > > whenever they access corrupted FDT in the FIXMAP area.
> > >
> > > On RV64 systems, TASK_SIZE is set to fixed 256GB and no other
> > > areas
> > > happen to overlap so we don't see any FIXMAP area corruptions.
> > >
> > > This patch fixes FIXMAP area corruption on RV32 systems by
> > > setting
> > > TASK_SIZE to FIXADDR_START.
> >
> > This part -- the TASK_SIZE change -- makes sense to me.
> >
> > However, the patch also changes FIXADDR_SIZE to be defined in terms
> > of
> > page table-related constants. Previously, FIXADDR_SIZE was based
> > on
> > __end_of_fixed_addresses, as it is for most other architectures.
> > The part
> > of the patch that changes FIXADDR_SIZE seems unrelated to the
> > actual fix.
> >
> > If that's indeed the case -- that the change to FIXADDR_SIZE is
> > unrelated
> > from the fix -- could you please split that into a separate patch,
> > with a
> > description of the rationale? I think I understand why you're
> > proposing
> > it, but it seems odd to explicitly connect it to page table-related
> > constants, rather than the contents of "enum fixed_addresses", and
> > I'm
> > reluctant to merge that part of this patch without a bit more
> > discussion.
>
> The FIXADDR_SIZE change is related to the TASK_SIZE requirement and
> it is not a separate change because:
>
> 1. TASK_SIZE must be evenly divisible by PGDIR_SIZE. The
> FIXADDR_START
> is defined as (FIXADDR_TOP - FIXADDR_SIZE). The original FIXADDR_SIZE
> defined in-terms of __end_of_fixed_addresses is not a multiple of
> PGDIR_SIZE
> hence it makes sense to make FIXADDR_SIZE as PGDIR_SIZE.
>
> 2. Let say we ignore point1 above then still we cannot continue to
> express
> FIXADDR_SIZE in-terms of __end_of_fixed_addresses because of cyclic
> header dependency where asm/fixmap.h includes asm/pgtable.h and
> __end_of_fixed_addresses is defined in asm/fixmap.h. We certainly
> need
> to move FIXADDR_TOP, FIXADDR_START, and FIXADDR_SIZE to
> asm/pgtable.h so that we can express TASK_SIZE as FIXADDR_START
> for RV32. If we don't simplify FIXADDR_SIZE then it will result in
> compile
> errors.
Ping!
Are we going to regress 32-bit support in 5.3?
Alistair
>
> Regards,
> Anup
>
> >
> > > We also move FIXADDR_TOP, FIXADDR_SIZE, and FIXADDR_START defines
> > > to
> > > asm/pgtable.h so that we can avoid cyclic header includes.
> > >
> > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > Tested-by: Alistair Francis <alistair.francis@wdc.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > > Changes since v1:
> > > - Drop braces from "#define FIXADDR_TOP"
> > > ---
> > > arch/riscv/include/asm/fixmap.h | 4 ----
> > > arch/riscv/include/asm/pgtable.h | 12 ++++++++++--
> > > 2 files changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/fixmap.h
> > > b/arch/riscv/include/asm/fixmap.h
> > > index 9c66033c3a54..161f28d04a07 100644
> > > --- a/arch/riscv/include/asm/fixmap.h
> > > +++ b/arch/riscv/include/asm/fixmap.h
> > > @@ -30,10 +30,6 @@ enum fixed_addresses {
> > > __end_of_fixed_addresses
> > > };
> > >
> > > -#define FIXADDR_SIZE (__end_of_fixed_addresses *
> > > PAGE_SIZE)
> > > -#define FIXADDR_TOP (VMALLOC_START)
> > > -#define FIXADDR_START (FIXADDR_TOP -
> > > FIXADDR_SIZE)
> > > -
> > > #define FIXMAP_PAGE_IO PAGE_KERNEL
> > >
> > > #define __early_set_fixmap __set_fixmap
> > > diff --git a/arch/riscv/include/asm/pgtable.h
> > > b/arch/riscv/include/asm/pgtable.h
> > > index a364aba23d55..c24a083b3e12 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -420,14 +420,22 @@ static inline void pgtable_cache_init(void)
> > > #define VMALLOC_END (PAGE_OFFSET - 1)
> > > #define VMALLOC_START (PAGE_OFFSET - VMALLOC_SIZE)
> > >
> > > +#define FIXADDR_TOP VMALLOC_START
> > > +#ifdef CONFIG_64BIT
> > > +#define FIXADDR_SIZE PMD_SIZE
> > > +#else
> > > +#define FIXADDR_SIZE PGDIR_SIZE
> > > +#endif
> > > +#define FIXADDR_START (FIXADDR_TOP - FIXADDR_SIZE)
> > > +
> > > /*
> > > - * Task size is 0x4000000000 for RV64 or 0xb800000 for RV32.
> > > + * Task size is 0x4000000000 for RV64 or 0x9fc00000 for RV32.
> > > * Note that PGDIR_SIZE must evenly divide TASK_SIZE.
> > > */
> > > #ifdef CONFIG_64BIT
> > > #define TASK_SIZE (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > #else
> > > -#define TASK_SIZE VMALLOC_START
> > > +#define TASK_SIZE FIXADDR_START
> > > #endif
> > >
> > > #include <asm-generic/pgtable.h>
> > > --
> > > 2.17.1
> > >
> >
> > - Paul
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems
2019-08-27 2:41 ` Anup Patel
2019-08-28 22:32 ` Alistair Francis
@ 2019-08-28 22:33 ` Paul Walmsley
1 sibling, 0 replies; 6+ messages in thread
From: Paul Walmsley @ 2019-08-28 22:33 UTC (permalink / raw)
To: Anup Patel
Cc: Palmer Dabbelt, Anup Patel, linux-kernel, Christoph Hellwig,
Atish Patra, Alistair Francis, linux-riscv
On Tue, 27 Aug 2019, Anup Patel wrote:
> On Tue, Aug 27, 2019 at 5:43 AM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >
> > On Mon, 19 Aug 2019, Anup Patel wrote:
> >
> > > Currently, various virtual memory areas of Linux RISC-V are organized
> > > in increasing order of their virtual addresses is as follows:
> > > 1. User space area (This is lowest area and starts at 0x0)
> > > 2. FIXMAP area
> > > 3. VMALLOC area
> > > 4. Kernel area (This is highest area and starts at PAGE_OFFSET)
> > >
> > > The maximum size of user space aread is represented by TASK_SIZE.
> > >
> > > On RV32 systems, TASK_SIZE is defined as VMALLOC_START which causes the
> > > user space area to overlap the FIXMAP area. This allows user space apps
> > > to potentially corrupt the FIXMAP area and kernel OF APIs will crash
> > > whenever they access corrupted FDT in the FIXMAP area.
> > >
> > > On RV64 systems, TASK_SIZE is set to fixed 256GB and no other areas
> > > happen to overlap so we don't see any FIXMAP area corruptions.
> > >
> > > This patch fixes FIXMAP area corruption on RV32 systems by setting
> > > TASK_SIZE to FIXADDR_START.
> >
> > This part -- the TASK_SIZE change -- makes sense to me.
> >
> > However, the patch also changes FIXADDR_SIZE to be defined in terms of
> > page table-related constants. Previously, FIXADDR_SIZE was based on
> > __end_of_fixed_addresses, as it is for most other architectures. The part
> > of the patch that changes FIXADDR_SIZE seems unrelated to the actual fix.
> >
> > If that's indeed the case -- that the change to FIXADDR_SIZE is unrelated
> > from the fix -- could you please split that into a separate patch, with a
> > description of the rationale? I think I understand why you're proposing
> > it, but it seems odd to explicitly connect it to page table-related
> > constants, rather than the contents of "enum fixed_addresses", and I'm
> > reluctant to merge that part of this patch without a bit more discussion.
>
> The FIXADDR_SIZE change is related to the TASK_SIZE requirement and
> it is not a separate change because:
>
> 1. TASK_SIZE must be evenly divisible by PGDIR_SIZE. The FIXADDR_START
> is defined as (FIXADDR_TOP - FIXADDR_SIZE). The original FIXADDR_SIZE
> defined in-terms of __end_of_fixed_addresses is not a multiple of PGDIR_SIZE
> hence it makes sense to make FIXADDR_SIZE as PGDIR_SIZE.
>
> 2. Let say we ignore point1 above then still we cannot continue to express
> FIXADDR_SIZE in-terms of __end_of_fixed_addresses because of cyclic
> header dependency where asm/fixmap.h includes asm/pgtable.h and
> __end_of_fixed_addresses is defined in asm/fixmap.h. We certainly need
> to move FIXADDR_TOP, FIXADDR_START, and FIXADDR_SIZE to
> asm/pgtable.h so that we can express TASK_SIZE as FIXADDR_START
> for RV32. If we don't simplify FIXADDR_SIZE then it will result in compile
> errors.
It would be better if we could stick to the same approach used by other
Linux architectures. That keeps things consistent across architectures.
However, in the short term, as you note, the header file changes to get to
that point are likely to be too intense for the late -rc series that we're
in.
So, queued for v5.3-rc7. Thanks very much for the speedy fix,
- Paul
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-28 22:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 5:14 [PATCH v2] RISC-V: Fix FIXMAP area corruption on RV32 systems Anup Patel
2019-08-26 6:44 ` Christoph Hellwig
2019-08-27 0:13 ` Paul Walmsley
2019-08-27 2:41 ` Anup Patel
2019-08-28 22:32 ` Alistair Francis
2019-08-28 22:33 ` Paul Walmsley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).