linux-kernel - Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in rv_s

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Aug 2023 09:08:38 -0700
From:   Luke Nelson <lukenels@...washington.edu>
To:     Pu Lehui <pulehui@...wei.com>
Cc:     bpf@...r.kernel.org, kernel test robot <lkp@...el.com>,
        Xi Wang <xi.wang@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>,
        Yonghong Song <yonghong.song@...ux.dev>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in
 rv_s_insn

>>  static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
>>  {
>> - u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
>> + u32 imm11_5 = (imm11_0 >> 5) & 0x7f, imm4_0 = imm11_0 & 0x1f;
> 
> Hi Luke,
> 
> keep u8 and add 0x7f explicit mask should work. I ran the repro case and it can silence the warning.
> 
>> 
>>   return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
>>   (imm4_0 << 7) | opcode;

That does fix the warning, but I think explicitly declaring imm11_5
as u32 is more clear here than the current code which relies on
implicit promotion of imm11_5 from u8 to signed int in the expression
(imm11_5 << 25).

Because of the promotion to signed int, (imm11_5 << 25) is technically
signed overflow and undefined behavior whenever the shift changes
the value in the sign bit. In practice, this isn't an issue; both
because the kernel is compiled with -fno-strict-overflow, but also
because GCC documentation explicitly states that "GCC does not use
the latitude given in C99 and C11 only to treat certain aspects of
signed '<<' as undefined" [1].

Though it may not be an issue in practice, since I'm touching this
line anyways to fix the warning, I think it makes sense to update
the type of imm11_5 to be u32 at the same time.

> Nit: maybe use "riscv, bpf" for the subject will look nice for the riscv-bpf git log tree.

Sure, I can send out a new revision with an updated subject line.

- Luke

[1]: https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html