Re: [PATCH 1/2] riscv: prevent null-pointer dereference with sbi_remote_fence_i

[Heiko Stuebner]

Hi Atish,

Am Dienstag, 14. Dezember 2021, 02:51:17 CET schrieb Atish Patra:
> On Mon, Dec 13, 2021 at 3:21 AM Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> >
> > The callback used inside sbi_remote_fence_i is set at sbi probe time
> > to the needed variant. Before that it is a NULL pointer.
> >
> > The selection between using sbi_remote_fence_i or ipi_remote_fence_i
> > right now is solely done on the presence of the RISCV_SBI config option.
> >
> > On a multiplatform kernel, SBI will probably always be built in, but
> > in the future not all machines using that kernel may have SBI
> > on them, so in some cases this setup can lead to NULL pointer dereferences.
> >
> 
> I didn't quite understand this. The only platforms without SBI are
> M-mode Linux based platforms.
> All other platforms will require SBI if the kernel is running on
> supervisor mode.

Ok ... but imagine a single-core board starting in S-mode.
Not doing smp, kvm, etc.

So I can very well imagine the possibility that people
might start it without an sbi instance running ;-) .


> They may not need SBI IPI or SBI RFENCE extensions if an alternate
> mechanism(ACLINT SSWI or AIA IMSIC)
> is already available in hardware. IIRC, Anup's aclint & aia series
> already takes care of that.
> 
> > Also if in future one of the flush_icache_all / flush_icache_mm functions
> > gets called earlier in the boot process - before sbi_init - this would
> > trigger the issue.
> >
> 
> sbi_init is called before setup_smp. Do you think there is a use case where
> flush_icache_xxx can be called before smp initialization ?

I ran into the issue while experimenting with doing alternatives patching
earlier. Works fine, but the flush_icache call then runs into that null-ptr.


Heiko


> > To prevent this, add a default __sbi_rfence_none returning an error code
> > and adapt the callers to check for an error from the remote fence
> > to then fall back to the other method.
> >
> 
> Having said that, it is always good to have guards to avoid NULL
> pointer dereferences.
> However, the commit text may be updated based on the above questions.
> 
> > Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> > ---
> >  arch/riscv/kernel/sbi.c    | 10 +++++++++-
> >  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++--------
> >  2 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 7402a417f38e..69d0a96b97d0 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,11 +14,19 @@
> >  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> >  EXPORT_SYMBOL(sbi_spec_version);
> >
> > +static int __sbi_rfence_none(int fid, const unsigned long *hart_mask,
> > +                            unsigned long start, unsigned long size,
> > +                            unsigned long arg4, unsigned long arg5)
> > +{
> > +       return -EOPNOTSUPP;
> > +}
> > +
> >  static void (*__sbi_set_timer)(uint64_t stime) __ro_after_init;
> >  static int (*__sbi_send_ipi)(const unsigned long *hart_mask) __ro_after_init;
> >  static int (*__sbi_rfence)(int fid, const unsigned long *hart_mask,
> >                            unsigned long start, unsigned long size,
> > -                          unsigned long arg4, unsigned long arg5) __ro_after_init;
> > +                          unsigned long arg4, unsigned long arg5)
> > +                          __ro_after_init = __sbi_rfence_none;
> >
> >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> >                         unsigned long arg1, unsigned long arg2,
> > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 89f81067e09e..128e23c094ea 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -16,11 +16,15 @@ static void ipi_remote_fence_i(void *info)
> >
> >  void flush_icache_all(void)
> >  {
> > +       int ret = -EINVAL;
> > +
> >         local_flush_icache_all();
> >
> >         if (IS_ENABLED(CONFIG_RISCV_SBI))
> > -               sbi_remote_fence_i(NULL);
> > -       else
> > +               ret = sbi_remote_fence_i(NULL);
> > +
> > +       /* fall back to ipi_remote_fence_i if sbi failed or not available */
> > +       if (ret)
> >                 on_each_cpu(ipi_remote_fence_i, NULL, 1);
> >  }
> >  EXPORT_SYMBOL(flush_icache_all);
> > @@ -66,13 +70,18 @@ void flush_icache_mm(struct mm_struct *mm, bool local)
> >                  * with flush_icache_deferred().
> >                  */
> >                 smp_mb();
> > -       } else if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > -               cpumask_t hartid_mask;
> > -
> > -               riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > -               sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> >         } else {
> > -               on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> > +               int ret = -EINVAL;
> > +
> > +               if (IS_ENABLED(CONFIG_RISCV_SBI)) {
> > +                       cpumask_t hartid_mask;
> > +
> > +                       riscv_cpuid_to_hartid_mask(&others, &hartid_mask);
> > +                       ret = sbi_remote_fence_i(cpumask_bits(&hartid_mask));
> 
> This API is removed in sparse hartid series[1]. You may want to rebase
> on top of it.
> 
> [1] https://patchwork.kernel.org/project/linux-riscv/list/?series=590195
> 
> > +               }
> > +
> > +               if (ret)
> > +                       on_each_cpu_mask(&others, ipi_remote_fence_i, NULL, 1);
> >         }
> >
> >         preempt_enable();
> > --
> > 2.30.2
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> 
> 
>