Re: [PATCH bpf 1/1] bpf: crypto: check params size before reading reserved fields

[Yuqi Xu <xuyuqiabc@gmail.com>]

Hi all,

This note is about a Sashiko finding on the already-applied patch
(a11212910cf0); that review was not posted to the list.  The note
below is analysis only; I am not asking to change the merged patch.

> The `algo` string in `struct bpf_crypto_params` is not checked for
> null-termination before being passed to the crypto API. A BPF program
> can fill this array and subsequent fields with non-null bytes, causing
> `vsnprintf` in `request_module` to read out-of-bounds, potentially
> resulting in a kernel panic or leaking kernel memory to user-space.
> locations: kernel/bpf/crypto.c:165 `bpf_crypto_ctx_create`;
> kernel/bpf/crypto.c:185

This is a valid finding.  It is also pre-existing and independent of the
params__sz out-of-bounds read that this patch fixes.

The kfunc's __sz annotation only bounds-checks the buffer: the verifier
calls check_mem_size_reg() on the params / params__sz pair and checks
that params__sz bytes of the pointer are readable.  It neither zeroes
nor NUL-terminates the buffer.  After the size check in
bpf_crypto_ctx_create(), params__sz == sizeof(struct bpf_crypto_params)
== 408, so algo[] (offset 16, 128 bytes, kernel/bpf/crypto.c:33) lies
inside the validated region, but nothing guarantees a NUL anywhere in
it.  A program can pass reserved[0] = reserved[1] = 0 (which the
function requires) and fill algo[] and the trailing fields with non-NUL
bytes.

type->has_algo(params->algo) (kernel/bpf/crypto.c:165) then reaches an
unbounded read:

  bpf_crypto_lskcipher_has_algo()
    crypto_has_skcipher()          crypto/skcipher.c:666
      crypto_type_has_alg()        crypto/algapi.c:1039
        crypto_find_alg()          crypto/api.c:535
          crypto_alg_mod_lookup()  crypto/api.c:338
            crypto_larval_lookup()  crypto/api.c:290
              request_module("crypto-%s", name)   crypto/api.c:303
                vsnprintf(module_name, MODULE_NAME_LEN, fmt, args)
                                                  kernel/module/kmod.c:150

The "%s" conversion has no precision, so vsnprintf() does a plain
strlen() on name; it keeps reading through key[], key_len and authsize
and past the 408-byte region that the verifier validated, until it finds
a zero byte.  KASAN reports a slab-out-of-bounds read if the object ends
before the next zero.  That is the extent of the bug: the OOB is only
that strlen()/string_nocheck walk past the 408-byte region.  An
unterminated algo makes vsnprintf()'s "%s" read unbounded, but when the
return value is >= MODULE_NAME_LEN, kmod.c returns -ENAMETOOLONG and
does not reach call_modprobe() or the usermode helper.

The finding also cites kernel/bpf/crypto.c:185.  That line is a blank
line, not a second use of algo; if (!ctx) after kzalloc is at 181.
The nearby second use of params->algo is type->alloc_tfm() at 187;
185 is a near miss for that line.  A 128-byte all-non-NUL algo cannot
match any already-loaded cra_name, so has_algo() returns false,
bpf_crypto_ctx_create() sets -EOPNOTSUPP, and type->alloc_tfm() is
not reached.

This is only reachable from BPF_PROG_TYPE_SYSCALL (the
crypt_init_kfunc_set registration at kernel/bpf/crypto.c:393), which
requires CAP_BPF, so it is not an unprivileged attack.  Still, the
verifier-validated buffer is the trust boundary, and reading past it is
a bug regardless.

Relationship to this patch: the patch only moves the params__sz
comparison ahead of the reserved[] reads.  The algo[] termination
problem exists identically before and after it, so it is a separate root
cause and is neither fixed nor worsened here.

A follow-up would validate that the strings are terminated before
handing them to the crypto API, for example:

  if (strnlen(params->algo, sizeof(params->algo)) == sizeof(params->algo) ||
      strnlen(params->type, sizeof(params->type)) == sizeof(params->type)) {
          *err = -EINVAL;
          return NULL;
  }

(memchr(params->algo, '\0', sizeof(params->algo)) is equivalent.)
params is const, so writing
params->algo[sizeof(params->algo) - 1] = '\0' would modify the
caller's buffer; explicitly rejecting an unterminated name is cleaner
than silently truncating it.  params->type[] is only compared with
strcmp() against the short, NUL-terminated registered type names, so
it cannot drive the unbounded read, but validating both keeps them
consistent.

That would be a separate patch (Fixes: 3e1c6f35409f), not a change to
the already-merged commit.  Happy to send it if you want it.

Thanks,
Yuqi Xu