Re: [PATCH] riscv, bpf: Emit fence.i for BPF_NOSPEC

[Luis Gerhorst <luis.gerhorst@fau.de>]

Lukas Gerlach <lukas.gerlach@cispa.de> writes:

> Regarding bpf_jit_bypass_spec_v1/v4(): currently this is per-architecture.
> What we need is per-processor granularity, so we can disable mitigations
> on in-order cores and keep them enabled on vulnerable out-of-order processors.

Yes, sorry this was unclear. I just wanted to point out that once you
have that infrastructure you can implement them as follows:

bpf_jit_bypass_spec_v1/v4() {
  if (RV_CPU_IN_ORDER())
    return true;
  else
    return false;
}

But you are right that the definition of RV_CPU_IN_ORDER() is still
missing of course.

> Regarding fence.i being an extension: all RISC-V processors supported by the
> kernel that are vulnerable to these attacks support this instruction.

I am not sure if I misunderstand the "that are vulnerable to these
attacks".

If you mean that vulnerable CPUs always have fence.i: What if a CPU
still does not support the instruction (no matter if it is vulnerable or
not)?

If I understand the RISC-V JIT correctly, it will then still generate
fence.i with this patch. When this eBPF program is then invoked the
CPU will panic upon reaching the fence.i which is a invalid opcode as
far as this CPU is concerned. Or is there some ifdef I am missing here?

I would assume you need something like this:

case BPF_ST | BPF_NOSPEC:
  if (riscv_has_extension_likely(RISCV_ISA_EXT_ZIFENCEI))
    emit_fence_i(ctx);
  break;

Or is there some guarantee that this extension is always available? I
read [3] as implying that this is no longer the case with the 2.0
instruction set for unprivileged. Does this also mean it is an optional
extension for the privileged ISA?

RISC-V cpufeature.c had this in [2]:

		/*
		 * Linux requires the following extensions, so we may as well
		 * always set them.
		 */
		set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
		set_bit(RISCV_ISA_EXT_ZIFENCEI, this_isa);

But as of [1] it was changed to:

		if (acpi_disabled) {
			set_bit(RISCV_ISA_EXT_ZICSR, source_isa);
			set_bit(RISCV_ISA_EXT_ZIFENCEI, source_isa);

So based on that I would assume fence.i may not always be supported.

But I also found that 921ebd8f2c08 ("RISC-V: Allow userspace to flush
the instruction cache") seems to assume that fence.i always works (see
local_flush_icache_all() which I assume runs in the kernel).

Even if the CPU is known vulnerable but does not support the extension,
it will be better not to generate the instruction. If we still want to
do something about these CPUs, maybe add a warning message to advise the
user that unpriv eBPF should definately be kept disabled on this CPU?

[1] https://lore.kernel.org/all/20230711224600.10879-1-palmer@rivosinc.com/
[2] https://lore.kernel.org/all/20230607-nest-collision-5796b6be8be6@spud/
[3] https://docs.riscv.org/reference/isa/unpriv/zifencei.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv