netdev - Re: [PATCH v6 bpf-next 3/6] bpf: add bpf_snprintf

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Sep 2020 17:42:29 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andriin@...com, yhs@...com,
        linux@...musvillemoes.dk, andriy.shevchenko@...ux.intel.com,
        pmladek@...e.com, kafai@...com, songliubraving@...com,
        john.fastabend@...il.com, kpsingh@...omium.org, shuah@...nel.org,
        rdna@...com, scott.branden@...adcom.com, quentin@...valent.com,
        cneirabustos@...il.com, jakub@...udflare.com, mingo@...hat.com,
        rostedt@...dmis.org, bpf@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        acme@...nel.org
Subject: Re: [PATCH v6 bpf-next 3/6] bpf: add bpf_snprintf_btf helper

On Wed, Sep 23, 2020 at 06:46:25PM +0100, Alan Maguire wrote:
> +
> +static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
> +				  u64 flags, const struct btf **btf,
> +				  s32 *btf_id)
> +{
> +	u8 btf_kind = BTF_KIND_TYPEDEF;
> +	char type_name[KSYM_NAME_LEN];
> +	const struct btf_type *t;
> +	const char *btf_type;
> +	int ret;
> +
> +	if (unlikely(flags & ~(BTF_F_ALL)))
> +		return -EINVAL;
> +
> +	if (btf_ptr_size != sizeof(struct btf_ptr))
> +		return -EINVAL;
> +
> +	*btf = bpf_get_btf_vmlinux();
> +
> +	if (IS_ERR_OR_NULL(*btf))
> +		return PTR_ERR(*btf);
> +
> +	if (ptr->type != NULL) {
> +		ret = copy_from_kernel_nofault(type_name, ptr->type,
> +					       sizeof(type_name));

nofault copy from bpf program global data... hmm...
I guess that works, but...

> +		if (ret)
> +			return ret;
> +
> +		btf_type = type_name;
> +
> +		if (strncmp(btf_type, "struct ", strlen("struct ")) == 0) {
> +			btf_kind = BTF_KIND_STRUCT;
> +			btf_type += strlen("struct ");
> +		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0) {
> +			btf_kind = BTF_KIND_UNION;
> +			btf_type += strlen("union ");
> +		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
> +			btf_kind = BTF_KIND_ENUM;
> +			btf_type += strlen("enum ");
> +		}
> +
> +		if (strlen(btf_type) == 0)
> +			return -EINVAL;
> +
> +		/* Assume type specified is a typedef as there's not much
> +		 * benefit in specifying int types other than wasting time
> +		 * on BTF lookups; we optimize for the most useful path.
> +		 *
> +		 * Fall back to BTF_KIND_INT if this fails.
> +		 */
> +		*btf_id = btf_find_by_name_kind(*btf, btf_type, btf_kind);
> +		if (*btf_id < 0)
> +			*btf_id = btf_find_by_name_kind(*btf, btf_type,
> +							BTF_KIND_INT);

with all that fragility...

> +	} else if (ptr->type_id > 0)
> +		*btf_id = ptr->type_id;

since __builtin_btf_type_id() landed in llvm in February and it works
may be support type_id only?

Manually specifying type name as a string is error prone.
Plus that copy_from_kernel... which is doing copy from bpf prog.
I slept on it, but still feels unclean.
May be do type_id only for now and if we really really need string types
we can add it later after initial patches land?