Re: [PATCH bpf-next v8 0/4] Add cryptographic hash and signature verification kfuncs to BPF

[Felix Maurer <fmaurer@redhat.com>]

Hi Daniel,

Sorry for taking long to get back to you, I've been on vacation for a
while.

> It would also be helpful to hear about what your use case is.

I'll put this one first, as it probably informs my thinking quite a bit.
My use case is accessing hash functions from the networking bpf hooks,
mostly XDP and tc. I'm less interested in the cryptographic hash
functions (although I think we should support them as well), but
checksums like CRC32c which have efficient implementations in the kernel
and are not straightforward to implement in bpf.

> > Taking an initial look at your hashing patches, I'm wondering: the usual
> > interface to hash/digest algorithms is to have three functions: an
> > init() function to set up state
>
> Doesn't bpf_crypto_ctx_create already provide the initialization? I was
> trying to make that pattern work by adding the bpf_crypto_type_id to
> make the code a little more maintainable.

No, bpf_crypto_ctx_create() doesn't do the initialization I was
referring to, it only creates the tfm that is later used to do the
hashing. The actual init() of the hash function happens inside
crypto_shash_digest(). More details at the end of my reply.

> > an update() function that can be called  multiple times to hash new
> > bytes, and a finalize() function that creates the actual hash.
> > Depending on the algorithm, some of them (esp.  finalize) may be
> > no-ops. Often, a fourth function, like hash(), is provided
>
> I think the bpf_crypto_encrypt should cover that along with the
> bpf_crypto_hash in the first patch.

Not sure if I understand you correctly, bpf_crypto_encrypt() shouldn't
be callable for a hashing crypto context at all?

> > I think we should provide the same init/update/finalize interface in bpf
> > as well to make the API more flexible. That would require splitting out
> > the shash_desc from the (mostly static) context. But doing so would also
> > address the review comment from bpf-ci bot to patch 1. WDYT?
>
> I was trying to make things work with the existing bpf_crypto_ctx
> lifecycle. IIRC in the V1/V2 of the series there was a separate struct
> but it was suggested to integrate the changes into bpf_crypto_ctx.

Yes, I see that the idea is to integrate into the existing life cycle
and I appreciate that! But IIUC, they are subtly different at the
moment.  Just to be sure, the idea of your first two patches is to
provide an interface to shash that supports the do-all-at-once function
hash()/digest() (i.e., internally do init+update+finalize), pretty much
the equivalent of the existing encrypt() + decrypt() interface to
skcipher, right? I.e. not supporting each and every use case for now but
just the case where all data is available and we want to hash/encrypt/
decrypt in one go?

With that assumption, the current implementation for skcipher looks like
this: we have bpf_crypto_ctx_create(). It sets up a bpf_crypto_ctx with
the chosen algorithm, the right key, etc. It must be called from a
sleepable bpf program because it may require to load the kernel module
with the requested algorithm. The prepared bpf_crypto_ctx can be stored
in a map so it can be used from other (non-sleepable) bpf programs,
e.g., XDP programs.

When the bpf_crypto_ctx is used with encrypt()/decrypt(), both functions
have the siv argument. It's a dynptr to some memory, containing the IV
for algorithms that need it, but also has room for the *state* if the
algorithm needs it. This means, the state is provided (fresh) for each
invocation of encrypt/decrypt.

Your proposed implementation for shash, bpf_crypto_ctx_create() calls
bpf_crypto_shash_alloc_tfm() which creates the tfm (just as above, needs
to be in a sleepable bpf program to potentially load modules), but it
also allocates the shash_desc which has room for some algorithm state if
needed. This mixes the static, long-living, reusable tfm with the
per-invocation shash_desc. shash is safe to use with the same tfm
concurrently exactly because the state is stored in the shash_desc,
which must not be used concurrently. Think of the shash_desc to be the
equivalent of the siv for skcipher.

Therefore, I think the memory for shash_desc.__ctx should come from a
dynptr as well. Do you agree or did I miss something?

Btw, your bpf_crypto_shash_alloc_tfm() never sets shash_desc.__ctx, so
that's a bug anyways.

> Regarding the bpf-ci bot I think it's somewhat valid, but you could
> solve that by putting the bpf_crypto_ctx in a per CPU map or protecting
> it with a bpf spinlock.

Two things for that: for networking use cases, needing a lock because of
implementation details doesn't sound very intriguing. Plus, if per-cpu
or a lock are required, that's currently not enforced. This means an
invalid/unsafe bpf program could pass the verifier, right?

> If you want to give it a go feel free.

If you're still interested in pursuing this, please go ahead with your
work, I don't want to take it from you. Only if you're explicitly not
interested anymore, I'll go ahead and try to build something on top of
you work. Just let me know :)

Thanks,
   Felix