Re: [PATCH v4] crypto: riscv/poly1305 - import OpenSSL/CRYPTOGAMS implementation

[Sami Tolvanen <samitolvanen@google.com>]

On Tue, Jun 24, 2025 at 8:56 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 24, 2025 at 11:13:49AM +0200, Andy Polyakov wrote:
> > > > +.globl   poly1305_init
> > > > +.type    poly1305_init,\@function
> > > > +poly1305_init:
> > > > +#ifdef   __riscv_zicfilp
> > > > + lpad    0
> > > > +#endif
> > >
> > > The 'lpad' instructions aren't present in the upstream CRYPTOGAMS source.
> >
> > They are.
>
> I now see the latest version does have them.  However, the description of this
> patch explicitly states it was taken from CRYPTOGAMS commit
> 33fe84bc21219a16825459b37c825bf4580a0a7b.  Which is of course the one I looked
> at, and it did not have them.  So the patch description is wrong.
>
> > > If they are necessary, this addition needs to be documented.
> > >
> > > But they appear to be unnecessary.
> >
> > They are better be there if Control Flow Integrity is on. It's the same deal
> > as with endbranch instruction on Intel and hint #34 on ARM. It's possible
> > that the kernel never engages CFI for itself, in which case all the
> > mentioned instructions are executed as nop-s. But note that here they are
> > compiled conditionally, so that if you don't compile the kernel with
> > -march=..._zicfilp_..., then they won't be there.
>
> There appears to be no kernel-mode support for Zicfilp yet.  This would be the
> very first occurrence of the lpad instruction in the kernel source.

Of course, if the kernel actually ends up calling these functions
indirectly at some point, lpad alone isn't sufficient, we would need
to use SYM_TYPED_FUNC_START to emit CFI type information for them. I
assume if RISC-V gains kernel-mode Zicfilp support later, we would
have an arch-specific override for the SYM_TYPED_FUNC_START macro that
includes the lpad instruction, similarly to arm64 and BTI.

Also, if the kernel decides to use type-based landing pad labels for
finer-grained CFI, "lpad 0" isn't going to work anyway. Perhaps it
would make sense to just drop the lpad instruction in kernel builds
for now to avoid confusion?

Sami