| Message ID | 20190816114915.4648-1-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Fix FIXMAP area corruption on RV32 systems | expand |
On Fri, 2019-08-16 at 11:49 +0000, Anup Patel wrote: > Currently, the order of various virtual memory areas in increasing > order of virtual addresses is as follows: > 1. User space area > 2. FIXMAP area > 3. VMALLOC area > 4. Kernel area > > The user space area starts at 0x0 and it's maximum size 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> This fixes the RV32 issue. Tested-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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..9dd08a006a28 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>
> +#define FIXADDR_TOP (VMALLOC_START) Nit: no need for the braces, the definitions below don't use it either. > +#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 Mentioning the addresses is a little weird. IMHO this would be a much nicer place to explain the high-level memory layout, including maybe a little ASCII art. Also we could have one #ifdef CONFIG_64BIT for both related values. Last but not least instead of saying that something should be dividable it would be nice to have a BUILD_BUG_ON to enforce it. Either way we are late in the cycle, so I guess this is ok for now: Reviewed-by: Christoph Hellwig <hch@lst.de> But I'd love to see this area improved a little further as it is full of mine fields.
On Sun, Aug 18, 2019 at 11:49 PM Christoph Hellwig <hch@infradead.org> wrote: > > > +#define FIXADDR_TOP (VMALLOC_START) > > Nit: no need for the braces, the definitions below don't use it > either. Sure, I will update and send v2 soon. > > > +#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 > > Mentioning the addresses is a little weird. IMHO this would be > a much nicer place to explain the high-level memory layout, including > maybe a little ASCII art. Also we could have one #ifdef CONFIG_64BIT > for both related values. Last but not least instead of saying that > something should be dividable it would be nice to have a BUILD_BUG_ON > to enforce it. > > Either way we are late in the cycle, so I guess this is ok for now: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > But I'd love to see this area improved a little further as it is full > of mine fields. I agree with you. We also have Sparsemem and KASAN patches which touch virtual memory layout so it's important to have virtual memory layout documented clearly. I can add the required documentation as separate patch. I think the best place to add ASCII art would be asm/pgtable.h where all virtual memory related defines are placed. Suggestions?? Regards, Anup
On Sun, 18 Aug 2019 21:49:01 PDT (-0700), anup@brainfault.org wrote: > On Sun, Aug 18, 2019 at 11:49 PM Christoph Hellwig <hch@infradead.org> wrote: >> >> > +#define FIXADDR_TOP (VMALLOC_START) >> >> Nit: no need for the braces, the definitions below don't use it >> either. > > Sure, I will update and send v2 soon. > >> >> > +#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 >> >> Mentioning the addresses is a little weird. IMHO this would be >> a much nicer place to explain the high-level memory layout, including >> maybe a little ASCII art. Also we could have one #ifdef CONFIG_64BIT >> for both related values. Last but not least instead of saying that >> something should be dividable it would be nice to have a BUILD_BUG_ON >> to enforce it. >> >> Either way we are late in the cycle, so I guess this is ok for now: >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> >> But I'd love to see this area improved a little further as it is full >> of mine fields. > > I agree with you. We also have Sparsemem and KASAN patches which > touch virtual memory layout so it's important to have virtual memory layout > documented clearly. I can add the required documentation as separate patch. Documentation is great, but if we document something that is broken then it's still broken :) I think this needs to just be redone -- we keep running into issues here and fixing them, but there are probably more issues and it'll probably be faster to just think through the memory map than to keep fixing bugs as they crop up. This was one of the areas of the port I didn't rewrite as part of the upstream submission process, and as a result it's pretty crusty. > I think the best place to add ASCII art would be asm/pgtable.h where all > virtual memory related defines are placed. Suggestions??
On Mon, 2019-08-26 at 14:17 -0700, Palmer Dabbelt wrote: > On Sun, 18 Aug 2019 21:49:01 PDT (-0700), anup@brainfault.org wrote: > > On Sun, Aug 18, 2019 at 11:49 PM Christoph Hellwig < > > hch@infradead.org> wrote: > > > > +#define FIXADDR_TOP (VMALLOC_START) > > > > > > Nit: no need for the braces, the definitions below don't use it > > > either. > > > > Sure, I will update and send v2 soon. > > > > > > +#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 > > > > > > Mentioning the addresses is a little weird. IMHO this would be > > > a much nicer place to explain the high-level memory layout, > > > including > > > maybe a little ASCII art. Also we could have one #ifdef > > > CONFIG_64BIT > > > for both related values. Last but not least instead of saying > > > that > > > something should be dividable it would be nice to have a > > > BUILD_BUG_ON > > > to enforce it. > > > > > > Either way we are late in the cycle, so I guess this is ok for > > > now: > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > > > But I'd love to see this area improved a little further as it is > > > full > > > of mine fields. > > > > I agree with you. We also have Sparsemem and KASAN patches which > > touch virtual memory layout so it's important to have virtual > > memory layout > > documented clearly. I can add the required documentation as > > separate patch. > > Documentation is great, but if we document something that is broken > then it's > still broken :) I'm confused here. What is broken? Right now RV32 does not work with the 5.3 kernel and this patch fixes the regression. > > I think this needs to just be redone -- we keep running into issues > here and > fixing them, but there are probably more issues and it'll probably be > faster to > just think through the memory map than to keep fixing bugs as they > crop up. > This was one of the areas of the port I didn't rewrite as part of the > upstream > submission process, and as a result it's pretty crusty. I can't speak for rewriting the code, but that seems like something that should happen in the 5.4 merge window right? With RC6 already out this patch seems like the only option for 5.3. Unless we are just going to drop RV32 support from Linux in the 5.3 release? Alistair > > > I think the best place to add ASCII art would be asm/pgtable.h > > where all > > virtual memory related defines are placed. Suggestions??
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..9dd08a006a28 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>
Currently, the order of various virtual memory areas in increasing order of virtual addresses is as follows: 1. User space area 2. FIXMAP area 3. VMALLOC area 4. Kernel area The user space area starts at 0x0 and it's maximum size 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> --- arch/riscv/include/asm/fixmap.h | 4 ---- arch/riscv/include/asm/pgtable.h | 12 ++++++++++-- 2 files changed, 10 insertions(+), 6 deletions(-)