Re: [PATCH bpf-next v5 2/7] crypto: Add BPF hash algorithm type registration module

[XIAO WU <shawdoxwu@gmail.com>]

Hi,

> Can this cause a NULL pointer dereference when a BPF program calls
> bpf_crypto_ctx_create() with type="hash"?
>
> The bpf_crypto_ctx_create() function in kernel/bpf/crypto.c
> unconditionally calls type->setkey(), type->ivsize(), and
> type->statesize():
>
>     *err = type->setkey(ctx->tfm, params->key, params->key_len);
>     ...
>     ctx->siv_len = type->ivsize(ctx->tfm) + type->statesize(ctx->tfm);
>
> But bpf_crypto_shash_type does not implement these callbacks, leaving
> them as NULL.
>
> Note: This appears to be fixed later in the series by commit
> 76d771a64b50 ("bpf: Add hash kfunc for cryptographic hashing") which
> adds NULL checks before calling these function pointers. Should this
> commit be squashed with 76d771a64b50 to ensure each patch in the
> series is bisectable without introducing crashes?

Yes, confirmed.

I reproduced this on x86_64 with a sleepable BPF syscall program that
calls bpf_crypto_ctx_create() with:
- type = "hash"
- algo = "sha256"
- key_len = 1

That reaches the path where type->setkey/type->ivsize/type->statesize
are used without NULL checks for the hash type, and triggers the NULL
dereference as pointed out.

Below is the reproducer (inlined, no attachment):

--8<--
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <linux/bpf.h>
#include <linux/btf.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <unistd.h>

#ifndef __NR_bpf
#define __NR_bpf 321
#endif

#define LOG_BUF_SIZE (1 << 20)

static int bpf_sys(enum bpf_cmd cmd, union bpf_attr *attr, unsigned int
size) {
	return syscall(__NR_bpf, cmd, attr, size);
}

static size_t btf_type_size(const struct btf_type *t)
{
	__u16 vlen = BTF_INFO_VLEN(t->info);
	__u32 kind = BTF_INFO_KIND(t->info);
	size_t sz = sizeof(*t);

	switch (kind) {
	case BTF_KIND_INT: sz += sizeof(__u32); break;
	case BTF_KIND_ARRAY: sz += sizeof(struct btf_array); break;
	case BTF_KIND_STRUCT:
	case BTF_KIND_UNION: sz += (size_t)vlen * sizeof(struct
	btf_member); break; case BTF_KIND_ENUM: sz += (size_t)vlen *
	sizeof(struct btf_enum); break; case BTF_KIND_FUNC_PROTO: sz +=
	(size_t)vlen * sizeof(struct btf_param); break; case
	BTF_KIND_VAR: sz += sizeof(struct btf_var); break; case
	BTF_KIND_DATASEC: sz += (size_t)vlen * sizeof(struct
	btf_var_secinfo); break; case BTF_KIND_DECL_TAG: sz +=
	sizeof(struct btf_decl_tag); break; case BTF_KIND_ENUM64: sz +=
	(size_t)vlen * sizeof(struct btf_enum64); break; default:
	break; } return sz;
}

static int find_vmlinux_func_btf_id(const char *func_name)
{
	int fd = -1, ret = -1;
	struct stat st;
	unsigned char *blob = NULL;
	struct btf_header *hdr;
	char *types, *strs;
	__u32 off, id;
	ssize_t n;
	size_t got = 0;

	fd = open("/sys/kernel/btf/vmlinux", O_RDONLY);
	if (fd < 0) goto out;
	if (fstat(fd, &st) < 0 || st.st_size <= 0) goto out;

	blob = malloc(st.st_size);
	if (!blob) goto out;

	while (got < (size_t)st.st_size) {
		n = read(fd, blob + got, (size_t)st.st_size - got);
		if (n < 0) {
			if (errno == EINTR) continue;
			goto out;
		}
		if (n == 0) break;
		got += (size_t)n;
	}
	if (got < sizeof(*hdr)) goto out;

	hdr = (struct btf_header *)blob;
	if (hdr->magic != BTF_MAGIC || hdr->version != BTF_VERSION)
	goto out; if ((size_t)hdr->hdr_len + hdr->type_off +
	hdr->type_len > got) goto out; if ((size_t)hdr->hdr_len +
	hdr->str_off + hdr->str_len > got) goto out;

	types = (char *)blob + hdr->hdr_len + hdr->type_off;
	strs = (char *)blob + hdr->hdr_len + hdr->str_off;

	for (off = 0, id = 1; off < hdr->type_len; id++) {
		struct btf_type *t = (struct btf_type *)(types + off);
		const char *name = t->name_off ? (strs + t->name_off) :
	""; size_t sz = btf_type_size(t);

		if (sz == 0 || off + sz > hdr->type_len) goto out;
		if (BTF_INFO_KIND(t->info) == BTF_KIND_FUNC &&
		strcmp(name, func_name) == 0) { ret = (int)id;
			goto out;
		}
		off += sz;
	}

out:
	free(blob);
	if (fd >= 0) close(fd);
	return ret;
}

int main(void)
{
	int create_id =
find_vmlinux_func_btf_id("bpf_crypto_ctx_create"); int release_id =
find_vmlinux_func_btf_id("bpf_crypto_ctx_release"); if (create_id <= 0
|| release_id <= 0) { fprintf(stderr, "failed resolving BTF IDs:
create=%d release=%d\n", create_id, release_id); return 1;
	}

	enum { PARAMS_BASE = 424, ERR_BASE = 16, PARAMS_SIZE = 408 };

	struct bpf_insn insn[128];
	int pc = 0, off;

	for (off = -8; off >= -424; off -= 8)
		insn[pc++] = (struct bpf_insn){
			.code = BPF_ST | BPF_MEM | BPF_DW,
			.dst_reg = BPF_REG_10,
			.off = off,
			.imm = 0,
		};

	insn[pc++] = (struct bpf_insn){ .code = BPF_ST | BPF_MEM |
	BPF_W, .dst_reg = BPF_REG_10, .off = -PARAMS_BASE + 0, .imm =
	0x68736168 }; insn[pc++] = (struct bpf_insn){ .code = BPF_ST |
	BPF_MEM | BPF_W, .dst_reg = BPF_REG_10, .off = -PARAMS_BASE +
	16, .imm = 0x32616873 }; insn[pc++] = (struct bpf_insn){ .code
	= BPF_ST | BPF_MEM | BPF_W, .dst_reg = BPF_REG_10, .off =
	-PARAMS_BASE + 20, .imm = 0x00003635 }; insn[pc++] = (struct
	bpf_insn){ .code = BPF_ST | BPF_MEM | BPF_B, .dst_reg =
	BPF_REG_10, .off = -PARAMS_BASE + 144, .imm = 0x11 };
	insn[pc++] = (struct bpf_insn){ .code = BPF_ST | BPF_MEM |
	BPF_W, .dst_reg = BPF_REG_10, .off = -PARAMS_BASE + 400, .imm =
	1 };

	insn[pc++] = (struct bpf_insn){ .code = BPF_ALU64 | BPF_MOV |
	BPF_X, .dst_reg = BPF_REG_1, .src_reg = BPF_REG_10 };
	insn[pc++] = (struct bpf_insn){ .code = BPF_ALU64 | BPF_ADD |
	BPF_K, .dst_reg = BPF_REG_1, .imm = -PARAMS_BASE }; insn[pc++]
	= (struct bpf_insn){ .code = BPF_ALU64 | BPF_MOV | BPF_K,
	.dst_reg = BPF_REG_2, .imm = PARAMS_SIZE }; insn[pc++] =
	(struct bpf_insn){ .code = BPF_ALU64 | BPF_MOV | BPF_X,
	.dst_reg = BPF_REG_3, .src_reg = BPF_REG_10 }; insn[pc++] =
	(struct bpf_insn){ .code = BPF_ALU64 | BPF_ADD | BPF_K,
	.dst_reg = BPF_REG_3, .imm = -ERR_BASE };

	insn[pc++] = (struct bpf_insn){ .code = BPF_JMP | BPF_CALL,
	.src_reg = BPF_PSEUDO_KFUNC_CALL, .imm = create_id };

	insn[pc++] = (struct bpf_insn){ .code = BPF_JMP | BPF_JEQ |
	BPF_K, .dst_reg = BPF_REG_0, .off = 2, .imm = 0 }; insn[pc++] =
	(struct bpf_insn){ .code = BPF_ALU64 | BPF_MOV | BPF_X,
	.dst_reg = BPF_REG_1, .src_reg = BPF_REG_0 }; insn[pc++] =
	(struct bpf_insn){ .code = BPF_JMP | BPF_CALL, .src_reg =
	BPF_PSEUDO_KFUNC_CALL, .imm = release_id };

	insn[pc++] = (struct bpf_insn){ .code = BPF_ALU64 | BPF_MOV |
	BPF_K, .dst_reg = BPF_REG_0, .imm = 0 }; insn[pc++] = (struct
	bpf_insn){ .code = BPF_JMP | BPF_EXIT };

	char logbuf[LOG_BUF_SIZE];
	char lic[] = "GPL";
	union bpf_attr attr;
	memset(logbuf, 0, sizeof(logbuf));
	memset(&attr, 0, sizeof(attr));

	attr.prog_type = BPF_PROG_TYPE_SYSCALL;
	attr.prog_flags = BPF_F_SLEEPABLE;
	attr.insn_cnt = pc;
	attr.insns = (uint64_t)(uintptr_t)insn;
	attr.license = (uint64_t)(uintptr_t)lic;
	attr.log_level = 1;
	attr.log_size = sizeof(logbuf);
	attr.log_buf = (uint64_t)(uintptr_t)logbuf;
	memcpy(attr.prog_name, "poc_hash_bug", 12);

	int prog_fd = bpf_sys(BPF_PROG_LOAD, &attr, sizeof(attr));
	if (prog_fd < 0) {
		fprintf(stderr, "BPF_PROG_LOAD failed: errno=%d
	(%s)\n", errno, strerror(errno)); fprintf(stderr, "Verifier
	log:\n%s\n", logbuf); return 2;
	}

	union bpf_attr run;
	memset(&run, 0, sizeof(run));
	run.test.prog_fd = prog_fd;
	(void)bpf_sys(BPF_PROG_TEST_RUN, &run, sizeof(run));
	close(prog_fd);
	return 0;
}
--8<--

I agree this should be fixed at the patch granularity level. I will
squash the NULL-check fix into patch 2 in v6 so each patch remains
bisectable and does not introduce a crash window.

As this issue is already publicly discussed on bpf-next and raised by
CI review, replying on-list is appropriate.

Signed-off-by: XIAO WU <shawdoxwu@gmail.com>

Thanks