linux-kernel - Re: [PATCH v1 2/2] riscv: Clean up module relocations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANBLGcym5LER30xEVCmVU=6BTNTTLJDUL72DnVSB0V2MsBOwuA@mail.gmail.com>
Date:   Wed, 5 Aug 2020 19:28:29 +0200
From:   Emil Renner Berthing <kernel@...il.dk>
To:     Luke Nelson <lukenels@...washington.edu>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Björn Töpel <bjorn.topel@...il.com>,
        Zong Li <zong@...estech.com>, Andreas Schwab <schwab@...e.de>,
        LKML <linux-kernel@...r.kernel.org>, Xi Wang <xi.wang@...il.com>
Subject: Re: [PATCH v1 2/2] riscv: Clean up module relocations

On Thu, 30 Jul 2020 at 20:53, Luke Nelson <lukenels@...washington.edu> wrote:
>
> Thanks for the patch!
>
> > Also RISC-V has a number of instruction pairs to
> > generate 32bit immediates or jump/call offsets. Eg.:
> >
> > lui   rd, hi20
> > addi  rd, rd, lo12
>
> On RV64, both hi20 from lui and lo12 from addi are sign-extended to 64 bits.
> This means that there are some 32-bit signed offsets (in the range
> [2^31-2^11, 2^31-1])
> that are not encodable using (lui+addi), (auipc+jalr), etc. (see
> discussion at [1]).
>
> The following note is from the ISA manual:
> >>> Note that the set of address offsets that can be formed by pairing LUI with LD,
> >>> AUIPC with JALR, etc. in RV64I is [−2^31−2^11, 2^31−2^11−1].

Good point!

> The existing code and the new code both seem buggy if the offset happens to
> be a 32-bit int but falls outside of the encodable range.
>
> > +       if (offset != (s32)offset) {
> > [...]
> > +       if (offset != (s32)offset) {
> > [...]
>
> These checks should probably be replaced with something similar to
> what's used in the RV64 BPF JIT here: [2],
> except that this code should check if using RV32 or RV64, since the
> encodable range differs for each.

Yeah, I decided to leave these checks as they are to not change
everything at once, but I'll see if I can improve on those in the next
version.

> > My hope is that we can eventually factor out the code to generate
> > immediates and instructions so it can be reused both here, in the
> > jump-label code and in the bpf-jit code, but let's take it
> > one step at a time.
>
> This sounds great! Having fewer copies of RISC-V encoding logic around will
> hopefully decrease the likelihood of bugs :) Some other archs already
> have shared
> infrastructure for doing instruction encoding (e.g., in
> arch/arm64/kernel/insn.c);
> we should consider doing something similar for RISC-V.

Exactly this. I've even tried to copy the naming from arm64 as well.
I'll send a 2nd version of this after -rc1.

>
> - Luke Nelson
>
> [1]: https://groups.google.com/a/groups.riscv.org/forum/#!topic/isa-dev/bwWFhBnnZFQ
> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=489553dd13a88d8a882db10622ba8b9b58582ce4