Message ID | 20200907181659.92449-7-seanga2@gmail.com |
---|---|
State | Superseded |
Delegated to: | Andes |
Headers | show |
Series | riscv: Correctly handle IPIs already pending upon boot | expand |
On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: > > This allows code to use a construct like `if (gd & gd->...) { ... }` when > accessing the global data pointer. Without this change, it was possible for > a very early trap to cause _exit_trap to read arbitrary memory. This could > cause a second trap, preventing show_regs from being printed. > > Fixes: 7c6ca03eaed0035ca6676e9bc7f5f1dfcaae7e8f nits: wrong fixes tag format > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > arch/riscv/cpu/start.S | 20 +++++++++++++++++--- > arch/riscv/lib/interrupts.c | 3 ++- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > index ad18e746b6..59d3d7bbf4 100644 > --- a/arch/riscv/cpu/start.S > +++ b/arch/riscv/cpu/start.S > @@ -47,6 +47,13 @@ _start: > mv tp, a0 > mv s1, a1 > > + /* > + * Set the global data pointer to a known value in case we get a very > + * early trap. The global data pointer will be set its actual value only > + * after it has been initialized. > + */ > + mv gp, zero > + > la t0, trap_entry > csrw MODE_PREFIX(tvec), t0 > > @@ -85,10 +92,10 @@ call_board_init_f_0: > jal board_init_f_alloc_reserve > > /* > - * Set global data pointer here for all harts, uninitialized at this > - * point. > + * Save global data pointer for later. We don't set it here because it > + * is not initialized yet. > */ > - mv gp, a0 > + mv s0, a0 > > /* setup stack */ > #if CONFIG_IS_ENABLED(SMP) > @@ -135,6 +142,13 @@ wait_for_gd_init: > fence r, rw > bnez t1, 1b > > + /* > + * Set the global data pointer only when gd_t has been initialized. > + * This was already set by arch_setup_gd on the boot hart, but all other > + * harts' global data pointers gets set here. > + */ > + mv gp, s0 This is currently in the #ifndef CONFIG_XIP block. Should consider the #else branch? > + > /* register available harts in the available_harts mask */ > li t1, 1 > sll t1, t1, tp > diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c > index cd47e64487..ad870e98d8 100644 > --- a/arch/riscv/lib/interrupts.c > +++ b/arch/riscv/lib/interrupts.c > @@ -78,7 +78,8 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs) > > printf("EPC: " REG_FMT " RA: " REG_FMT " TVAL: " REG_FMT "\n", > epc, regs->ra, tval); > - if (gd->flags & GD_FLG_RELOC) > + /* Print relocation adjustments, but only if gd is initialized */ > + if (gd && gd->flags & GD_FLG_RELOC) > printf("EPC: " REG_FMT " RA: " REG_FMT " reloc adjusted\n\n", > epc - gd->reloc_off, regs->ra - gd->reloc_off); > > -- Regards, Bin
On 9/14/20 1:25 AM, Bin Meng wrote: > On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> This allows code to use a construct like `if (gd & gd->...) { ... }` when >> accessing the global data pointer. Without this change, it was possible for >> a very early trap to cause _exit_trap to read arbitrary memory. This could >> cause a second trap, preventing show_regs from being printed. >> >> Fixes: 7c6ca03eaed0035ca6676e9bc7f5f1dfcaae7e8f > > nits: wrong fixes tag format > >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> arch/riscv/cpu/start.S | 20 +++++++++++++++++--- >> arch/riscv/lib/interrupts.c | 3 ++- >> 2 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >> index ad18e746b6..59d3d7bbf4 100644 >> --- a/arch/riscv/cpu/start.S >> +++ b/arch/riscv/cpu/start.S >> @@ -47,6 +47,13 @@ _start: >> mv tp, a0 >> mv s1, a1 >> >> + /* >> + * Set the global data pointer to a known value in case we get a very >> + * early trap. The global data pointer will be set its actual value only >> + * after it has been initialized. >> + */ >> + mv gp, zero >> + >> la t0, trap_entry >> csrw MODE_PREFIX(tvec), t0 >> >> @@ -85,10 +92,10 @@ call_board_init_f_0: >> jal board_init_f_alloc_reserve >> >> /* >> - * Set global data pointer here for all harts, uninitialized at this >> - * point. >> + * Save global data pointer for later. We don't set it here because it >> + * is not initialized yet. >> */ >> - mv gp, a0 >> + mv s0, a0 >> >> /* setup stack */ >> #if CONFIG_IS_ENABLED(SMP) >> @@ -135,6 +142,13 @@ wait_for_gd_init: >> fence r, rw >> bnez t1, 1b >> >> + /* >> + * Set the global data pointer only when gd_t has been initialized. >> + * This was already set by arch_setup_gd on the boot hart, but all other >> + * harts' global data pointers gets set here. >> + */ >> + mv gp, s0 > > This is currently in the #ifndef CONFIG_XIP block. Should consider the > #else branch? gp is set by arch_setup_gd in board_init_f_init_reserve on the boot hart. Setting it here is only necessary for secondary harts. > >> + >> /* register available harts in the available_harts mask */ >> li t1, 1 >> sll t1, t1, tp >> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c >> index cd47e64487..ad870e98d8 100644 >> --- a/arch/riscv/lib/interrupts.c >> +++ b/arch/riscv/lib/interrupts.c >> @@ -78,7 +78,8 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs) >> >> printf("EPC: " REG_FMT " RA: " REG_FMT " TVAL: " REG_FMT "\n", >> epc, regs->ra, tval); >> - if (gd->flags & GD_FLG_RELOC) >> + /* Print relocation adjustments, but only if gd is initialized */ >> + if (gd && gd->flags & GD_FLG_RELOC) >> printf("EPC: " REG_FMT " RA: " REG_FMT " reloc adjusted\n\n", >> epc - gd->reloc_off, regs->ra - gd->reloc_off); >> >> -- > > Regards, > Bin >
On 9/14/20 9:03 AM, Sean Anderson wrote: > On 9/14/20 1:25 AM, Bin Meng wrote: >> On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: >>> >>> This allows code to use a construct like `if (gd & gd->...) { ... }` when >>> accessing the global data pointer. Without this change, it was possible for >>> a very early trap to cause _exit_trap to read arbitrary memory. This could >>> cause a second trap, preventing show_regs from being printed. >>> >>> Fixes: 7c6ca03eaed0035ca6676e9bc7f5f1dfcaae7e8f >> >> nits: wrong fixes tag format >> >>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>> --- >>> >>> arch/riscv/cpu/start.S | 20 +++++++++++++++++--- >>> arch/riscv/lib/interrupts.c | 3 ++- >>> 2 files changed, 19 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >>> index ad18e746b6..59d3d7bbf4 100644 >>> --- a/arch/riscv/cpu/start.S >>> +++ b/arch/riscv/cpu/start.S >>> @@ -47,6 +47,13 @@ _start: >>> mv tp, a0 >>> mv s1, a1 >>> >>> + /* >>> + * Set the global data pointer to a known value in case we get a very >>> + * early trap. The global data pointer will be set its actual value only >>> + * after it has been initialized. >>> + */ >>> + mv gp, zero >>> + >>> la t0, trap_entry >>> csrw MODE_PREFIX(tvec), t0 >>> >>> @@ -85,10 +92,10 @@ call_board_init_f_0: >>> jal board_init_f_alloc_reserve >>> >>> /* >>> - * Set global data pointer here for all harts, uninitialized at this >>> - * point. >>> + * Save global data pointer for later. We don't set it here because it >>> + * is not initialized yet. >>> */ >>> - mv gp, a0 >>> + mv s0, a0 >>> >>> /* setup stack */ >>> #if CONFIG_IS_ENABLED(SMP) >>> @@ -135,6 +142,13 @@ wait_for_gd_init: >>> fence r, rw >>> bnez t1, 1b >>> >>> + /* >>> + * Set the global data pointer only when gd_t has been initialized. >>> + * This was already set by arch_setup_gd on the boot hart, but all other >>> + * harts' global data pointers gets set here. >>> + */ >>> + mv gp, s0 >> >> This is currently in the #ifndef CONFIG_XIP block. Should consider the >> #else branch? > > gp is set by arch_setup_gd in board_init_f_init_reserve on the boot > hart. Setting it here is only necessary for secondary harts. Ok, I see what you mean. I guess I can set gp just before XIP jumps to secondary_hart_loop. Doing it that way is still vulnerable to pending IPIs, but not all cores should have that problem. Perhaps that limitation should be documented somewhere? In any case, I don't have an XIP core to test on (and there isn't a CI configuration either). --Sean >> >>> + >>> /* register available harts in the available_harts mask */ >>> li t1, 1 >>> sll t1, t1, tp >>> diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c >>> index cd47e64487..ad870e98d8 100644 >>> --- a/arch/riscv/lib/interrupts.c >>> +++ b/arch/riscv/lib/interrupts.c >>> @@ -78,7 +78,8 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs) >>> >>> printf("EPC: " REG_FMT " RA: " REG_FMT " TVAL: " REG_FMT "\n", >>> epc, regs->ra, tval); >>> - if (gd->flags & GD_FLG_RELOC) >>> + /* Print relocation adjustments, but only if gd is initialized */ >>> + if (gd && gd->flags & GD_FLG_RELOC) >>> printf("EPC: " REG_FMT " RA: " REG_FMT " reloc adjusted\n\n", >>> epc - gd->reloc_off, regs->ra - gd->reloc_off); >>> >>> -- >> >> Regards, >> Bin >> >
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index ad18e746b6..59d3d7bbf4 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -47,6 +47,13 @@ _start: mv tp, a0 mv s1, a1 + /* + * Set the global data pointer to a known value in case we get a very + * early trap. The global data pointer will be set its actual value only + * after it has been initialized. + */ + mv gp, zero + la t0, trap_entry csrw MODE_PREFIX(tvec), t0 @@ -85,10 +92,10 @@ call_board_init_f_0: jal board_init_f_alloc_reserve /* - * Set global data pointer here for all harts, uninitialized at this - * point. + * Save global data pointer for later. We don't set it here because it + * is not initialized yet. */ - mv gp, a0 + mv s0, a0 /* setup stack */ #if CONFIG_IS_ENABLED(SMP) @@ -135,6 +142,13 @@ wait_for_gd_init: fence r, rw bnez t1, 1b + /* + * Set the global data pointer only when gd_t has been initialized. + * This was already set by arch_setup_gd on the boot hart, but all other + * harts' global data pointers gets set here. + */ + mv gp, s0 + /* register available harts in the available_harts mask */ li t1, 1 sll t1, t1, tp diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c index cd47e64487..ad870e98d8 100644 --- a/arch/riscv/lib/interrupts.c +++ b/arch/riscv/lib/interrupts.c @@ -78,7 +78,8 @@ static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs) printf("EPC: " REG_FMT " RA: " REG_FMT " TVAL: " REG_FMT "\n", epc, regs->ra, tval); - if (gd->flags & GD_FLG_RELOC) + /* Print relocation adjustments, but only if gd is initialized */ + if (gd && gd->flags & GD_FLG_RELOC) printf("EPC: " REG_FMT " RA: " REG_FMT " reloc adjusted\n\n", epc - gd->reloc_off, regs->ra - gd->reloc_off);
This allows code to use a construct like `if (gd & gd->...) { ... }` when accessing the global data pointer. Without this change, it was possible for a very early trap to cause _exit_trap to read arbitrary memory. This could cause a second trap, preventing show_regs from being printed. Fixes: 7c6ca03eaed0035ca6676e9bc7f5f1dfcaae7e8f Signed-off-by: Sean Anderson <seanga2@gmail.com> --- arch/riscv/cpu/start.S | 20 +++++++++++++++++--- arch/riscv/lib/interrupts.c | 3 ++- 2 files changed, 19 insertions(+), 4 deletions(-)