netdev - Re: [PATCH v2 bpf-next 1/3] libbpf: support libbpf-provided extern variables

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20191213050739.xt4wnofdwf66gcrw@ast-mbp>
Date:   Thu, 12 Dec 2019 21:11:04 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andriin@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...com,
        daniel@...earbox.net, andrii.nakryiko@...il.com, kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 1/3] libbpf: support libbpf-provided extern
 variables

On Tue, Dec 10, 2019 at 10:50:00PM -0800, Andrii Nakryiko wrote:
> +static int set_ext_value_tri(struct extern_desc *ext, void *ext_val,
> +			     char value)
> +{
> +	switch (ext->type) {
> +	case EXT_BOOL:
> +		if (value == 'm') {
> +			pr_warn("extern %s=%c should be tristate or char\n",
> +				ext->name, value);
> +			return -EINVAL;
> +		}
> +		*(bool *)ext_val = value == 'y' ? true : false;

may be check for strict y/n ?

> +		break;
> +	case EXT_TRISTATE:
> +		if (value == 'y')
> +			*(enum libbpf_tristate *)ext_val = TRI_YES;
> +		else if (value == 'm')
> +			*(enum libbpf_tristate *)ext_val = TRI_MODULE;
> +		else /* value == 'n' */
> +			*(enum libbpf_tristate *)ext_val = TRI_NO;

same here ?

> +		break;
> +	case EXT_CHAR:
> +		*(char *)ext_val = value;
> +		break;
> +	case EXT_UNKNOWN:
> +	case EXT_INT:
> +	case EXT_CHAR_ARR:

why enumerate them when there is a default ?

> +static int set_ext_value_str(struct extern_desc *ext, void *ext_val,
> +			     const char *value)
> +{
> +	size_t len;
> +
> +	if (ext->type != EXT_CHAR_ARR) {
> +		pr_warn("extern %s=%s should char array\n", ext->name, value);
> +		return -EINVAL;
> +	}
> +
> +	len = strlen(value);
> +	if (value[len - 1] != '"') {
> +		pr_warn("extern '%s': invalid string config '%s'\n",
> +			ext->name, value);
> +		return -EINVAL;
> +	}
> +
> +	/* strip quotes */
> +	len -= 2;
> +	if (len + 1 > ext->sz) {
> +		pr_warn("extern '%s': too long string config %s (%zu bytes), up to %d expected\n",
> +			ext->name, value, len + 1, ext->sz);
> +		return -EINVAL;

may be print warning and truncate instead of hard error?

> +static bool is_ext_value_in_range(const struct extern_desc *ext, __u64 v)
> +{
> +	int bit_sz = ext->sz * 8;
> +
> +	if (ext->sz == 8)
> +		return true;
> +
> +	if (ext->is_signed)
> +		return v + (1ULL << (bit_sz - 1)) < (1ULL << bit_sz);
> +	else
> +		return (v >> bit_sz) == 0;

a comment would be helpful.

> +		ext_val = data + ext->data_off;
> +		value = sep + 1;
> +
> +		switch (*value) {
> +		case 'y': case 'n': case 'm':

I don't think config.gz has 'n', but it's good to have it here.

> -			} else if (strcmp(name, ".data") == 0) {
> +			} else if (strcmp(name, DATA_SEC) == 0) {
>  				obj->efile.data = data;
>  				obj->efile.data_shndx = idx;
> -			} else if (strcmp(name, ".rodata") == 0) {
> +			} else if (strcmp(name, RODATA_SEC) == 0) {

such cleanup changes should be in separate patch.

> +		if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> +			void *ext_val = data + ext->data_off;
> +			__u32 kver = get_kernel_version();
> +
> +			if (!kver) {
> +				pr_warn("failed to get kernel version\n");
> +				return -EINVAL;
> +			}
> +			err = set_ext_value_num(ext, ext_val, kver);

I think it should work when kern_ver is not 'int'.
Could you add a test for u64 ?
Or it will fail on big endian?