Message-ID: <0a5386c9-3dbd-1ed8-d94c-d866c6369743@fb.com>
Date:   Sun, 15 Sep 2019 00:16:48 +0000
From:   Yonghong Song <yhs@...com>
To:     KP Singh <kpsingh@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        James Morris <jmorris@...ei.org>,
        Kees Cook <keescook@...omium.org>,
        Thomas Garnier <thgarnie@...omium.org>,
        "Michael Halcrow" <mhalcrow@...gle.com>,
        Paul Turner <pjt@...gle.com>,
        Brendan Gregg <brendan.d.gregg@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Christian Brauner <christian@...uner.io>,
        Mickaël Salaün <mic@...ikod.net>,
        Florent Revest <revest@...omium.org>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        "Mauro Carvalho Chehab" <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        "Quentin Monnet" <quentin.monnet@...ronome.com>,
        Andrey Ignatov <rdna@...com>, "Joe Stringer" <joe@...d.net.nz>
Subject: Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value
 of an env variable



On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@...gle.com>

This patch cannot apply cleanly.

-bash-4.4$ git apply ~/p12.txt
error: patch failed: include/uapi/linux/bpf.h:2715
error: include/uapi/linux/bpf.h: patch does not apply
error: patch failed: tools/include/uapi/linux/bpf.h:2715
error: tools/include/uapi/linux/bpf.h: patch does not apply
-bash-4.4$

> 
> The helper returns the value of the environment variable in the buffer
> that is passed to it. If the var is set multiple times, the helper
> returns all the values as null separated strings.
> 
> If the buffer is too short for these values, the helper tries to fill it
> the best it can and guarantees that the value returned in the buffer
> is always null terminated. After the buffer is filled, the helper keeps
> counting the number of times the environment variable is set in the
> envp.
> 
> The return value of the helper is an u64 value which carries two pieces
> of information.
> 
>    * The upper 32 bits are a u32 value signifying the number of times
>      the environment variable is set in the envp.

Not sure how useful this 'upper 32' bit value is. What user expected to do?

Another option is to have upper 32 bits encode the required buffer size
to hold all values. This may cause some kind of user space action, e.g.,
to replace the program with new program with larger per cpu map value size?

>    * The lower 32 bits are a s32 value signifying the number of bytes
>      written to the buffer or an error code. >
> Since the value of the environment variable can be very long and exceed
> what can be allocated on the BPF stack, a per-cpu array can be used
> instead:
> 
> struct bpf_map_def SEC("maps") env_map = {
>          .type = BPF_MAP_TYPE_PERCPU_ARRAY,
>          .key_size = sizeof(u32),
>          .value_size = 4096,
>          .max_entries = 1,
> };

Could you use use map definition with SEC(".maps")?

> 
> SEC("prgrm")
> int bpf_prog1(void *ctx)
> {
>          u32 map_id = 0;
>          u64 times_ret;
>          s32 ret;
>          char name[48] = "LD_PRELOAD";

Reverse Christmas tree coding style, here and other places?

> 
>          char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
>          if (!map_value)
>                  return 0;
> 
>          // Read the lower 32 bits for the return value
>          times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
>          ret = times_ret & 0xffffffff;
>          if (ret < 0)
>                  return ret;
>          return 0;
> }
> 
> Signed-off-by: KP Singh <kpsingh@...gle.com>
> ---
>   include/uapi/linux/bpf.h                  |  42 ++++++-
>   security/krsi/ops.c                       | 129 ++++++++++++++++++++++
>   tools/include/uapi/linux/bpf.h            |  42 ++++++-
>   tools/testing/selftests/bpf/bpf_helpers.h |   3 +
>   4 files changed, 214 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 32ab38f1a2fe..a4ef07956e07 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2715,6 +2715,45 @@ union bpf_attr {
>    *		**-EPERM** if no permission to send the *sig*.
>    *
>    *		**-EAGAIN** if bpf program can try again.
> + *
> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> + *			size_t name_len, size_t buf_len)

This signature is not the same as the later
krsi_get_env_var(...) helper definition.
BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, 
n_size,
	  char *, dest, u32, size)

> + *	Description
> + *		This helper can be used as a part of the
> + *		process_execution hook of the KRSI LSM in
> + *		programs of type BPF_PROG_TYPE_KRSI.
> + *
> + *		The helper returns the value of the environment
> + *		variable with the provided "name" for process that's
> + *		going to be executed in the passed buffer, "buf". If the var
> + *		is set multiple times, the helper returns all
> + *		the values as null separated strings.
> + *
> + *		If the buffer is too short for these values, the helper
> + *		tries to fill it the best it can and guarantees that the value
> + *		returned in the buffer  is always null terminated.
> + *		After the buffer is filled, the helper keeps counting the number
> + *		of times the environment variable is set in the envp.
> + *
> + *	Return:
> + *
> + *		The return value of the helper is an u64 value
> + *		which carries two pieces of information:
> + *
> + *		   The upper 32 bits are a u32 value signifying
> + *		   the number of times the environment variable
> + *		   is set in the envp.
> + *		   The lower 32 bits are an s32 value signifying
> + *		   the number of bytes written to the buffer or an error code:
> + *
> + *		**-ENOMEM** if the kernel is unable to allocate memory
> + *			    for pinning the argv and envv.
> + *
> + *		**-E2BIG** if the value is larger than the size of the
> + *			   destination buffer. The higher bits will still
> + *			   the number of times the variable was set in the envp.

The -E2BIG is returned because buffer sizee is not big enough.
Another possible error code is -ENOSPC, which typically indicates
buffer size not big enough.

> + *
> + *		**-EINVAL** if name is not a NULL terminated string.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -2826,7 +2865,8 @@ union bpf_attr {
>   	FN(strtoul),			\
>   	FN(sk_storage_get),		\
>   	FN(sk_storage_delete),		\
> -	FN(send_signal),
> +	FN(send_signal),		\
> +	FN(krsi_get_env_var),
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>    * function eBPF program intends to call
> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> index 1f4df920139c..1db94dfaac15 100644
> --- a/security/krsi/ops.c
> +++ b/security/krsi/ops.c
> @@ -6,6 +6,8 @@
>   #include <linux/bpf.h>
>   #include <linux/security.h>
>   #include <linux/krsi.h>
> +#include <linux/binfmts.h>
> +#include <linux/highmem.h>
>   
>   #include "krsi_init.h"
>   #include "krsi_fs.h"
> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
>   	return false;
>   }
>   
> +static char *array_next_entry(char *array, unsigned long *offset,
> +			      unsigned long end)
> +{
> +	char *entry;
> +	unsigned long current_offset = *offset;
> +
> +	if (current_offset >= end)
> +		return NULL;
> +
> +	/*
> +	 * iterate on the array till the null byte is encountered
> +	 * and check for any overflows.
> +	 */
> +	entry = array + current_offset;
> +	while (array[current_offset]) {
> +		if (unlikely(++current_offset >= end))
> +			return NULL;
> +	}
> +
> +	/*
> +	 * Point the offset to the next element in the array.
> +	 */
> +	*offset = current_offset + 1;
> +
> +	return entry;
> +}
> +
> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> +		u32 n_size, u32 size)
> +{
> +	s32 ret = 0;
> +	u32 num_vars = 0;
> +	int i, name_len;
> +	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> +	int argc = bprm->argc;
> +	int envc = bprm->envc;
> +	unsigned long end = ctx->bprm_ctx.max_arg_offset;
> +	unsigned long offset = bprm->p % PAGE_SIZE;

why we need bprm->p % PAGE_SIZE instead of bprm->p?

> +	char *buf = ctx->bprm_ctx.arg_pages;
> +	char *curr_dest = dest;
> +	char *entry;
> +
> +	if (unlikely(!buf))
> +		return -ENOMEM;
> +
> +	for (i = 0; i < argc; i++) {
> +		entry = array_next_entry(buf, &offset, end);
> +		if (!entry)
> +			return 0;
> +	}
> +
> +	name_len = strlen(name);
> +	for (i = 0; i < envc; i++) {
> +		entry = array_next_entry(buf, &offset, end);
> +		if (!entry)
> +			return 0;

If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
we may skip the first entry?


> +
> +		if (!strncmp(entry, name, name_len)) {
> +			num_vars++;

There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.

> +
> +			/*
> +			 * There is no need to do further copying
> +			 * if the buffer is already full. Just count how many
> +			 * times the environment variable is set.
> +			 */
> +			if (ret == -E2BIG)
> +				continue;
> +
> +			if (entry[name_len] != '=')
> +				continue;
> +
> +			/*
> +			 * Move the buf pointer by name_len + 1
> +			 * (for the "=" sign)
> +			 */
> +			entry += name_len + 1;
> +			ret = strlcpy(curr_dest, entry, size);
> +
> +			if (ret >= size) {
> +				ret = -E2BIG;

Here, we have a partial copy. Should you instead nullify (memset) it as 
it is not really invalid one?

> +				continue;
> +			}
> +
> +			/*
> +			 * strlcpy just returns the length of the string copied.
> +			 * The remaining space needs to account for the added
> +			 * null character.
> +			 */
> +			curr_dest += ret + 1;
> +			size -= ret + 1;
> +			/*
> +			 * Update ret to be the current number of bytes written
> +			 * to the destination
> +			 */
> +			ret = curr_dest - dest;
> +		}
> +	}
> +
> +	return (u64) num_vars << 32 | (u32) ret;
> +}
> +
> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> +	  char *, dest, u32, size)
> +{
> +	char *name_end;
> +
> +	name_end = memchr(name, '\0', n_size);
> +	if (!name_end)
> +		return -EINVAL;
> +
> +	memset(dest, 0, size);
> +	return get_env_var(ctx, name, dest, n_size, size);
> +}
> +
> +static const struct bpf_func_proto krsi_get_env_var_proto = {
> +	.func = krsi_get_env_var,
> +	.gpl_only = true,
> +	.ret_type = RET_INTEGER,
> +	.arg1_type = ARG_PTR_TO_CTX,
> +	.arg2_type = ARG_PTR_TO_MEM,
> +	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
> +	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
> +	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
> +};
> +
>   BPF_CALL_5(krsi_event_output, void *, log,
>   	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
>   {
> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>   		return &bpf_map_lookup_elem_proto;
>   	case BPF_FUNC_get_current_pid_tgid:
>   		return &bpf_get_current_pid_tgid_proto;
> +	case BPF_FUNC_krsi_get_env_var:
> +		return &krsi_get_env_var_proto;
>   	case BPF_FUNC_perf_event_output:
>   		return &krsi_event_output_proto;
>   	default:
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 32ab38f1a2fe..a4ef07956e07 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2715,6 +2715,45 @@ union bpf_attr {
>    *		**-EPERM** if no permission to send the *sig*.
>    *
>    *		**-EAGAIN** if bpf program can try again.
> + *
> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> + *			size_t name_len, size_t buf_len)

Inconsistent helper definitions.

> + *	Description
> + *		This helper can be used as a part of the
> + *		process_execution hook of the KRSI LSM in
> + *		programs of type BPF_PROG_TYPE_KRSI.
> + *
> + *		The helper returns the value of the environment
> + *		variable with the provided "name" for process that's
> + *		going to be executed in the passed buffer, "buf". If the var
> + *		is set multiple times, the helper returns all
> + *		the values as null separated strings.
> + *
> + *		If the buffer is too short for these values, the helper
> + *		tries to fill it the best it can and guarantees that the value
> + *		returned in the buffer  is always null terminated.
> + *		After the buffer is filled, the helper keeps counting the number
> + *		of times the environment variable is set in the envp.
> + *
> + *	Return:
> + *
> + *		The return value of the helper is an u64 value
> + *		which carries two pieces of information:
> + *
> + *		   The upper 32 bits are a u32 value signifying
> + *		   the number of times the environment variable
> + *		   is set in the envp.
> + *		   The lower 32 bits are an s32 value signifying
> + *		   the number of bytes written to the buffer or an error code:
> + *
> + *		**-ENOMEM** if the kernel is unable to allocate memory
> + *			    for pinning the argv and envv.
> + *
> + *		**-E2BIG** if the value is larger than the size of the
> + *			   destination buffer. The higher bits will still
> + *			   the number of times the variable was set in the envp.
> + *
> + *		**-EINVAL** if name is not a NULL terminated string.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -2826,7 +2865,8 @@ union bpf_attr {
>   	FN(strtoul),			\
>   	FN(sk_storage_get),		\
>   	FN(sk_storage_delete),		\
> -	FN(send_signal),
> +	FN(send_signal),		\
> +	FN(krsi_get_env_var),
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>    * function eBPF program intends to call
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index f804f210244e..ecebdb772a9d 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
>   static int (*bpf_probe_read_str)(void *ctx, __u32 size,
>   				 const void *unsafe_ptr) =
>   	(void *) BPF_FUNC_probe_read_str;
> +static unsigned long long (*krsi_get_env_var)(void *ctx,
> +	void *name, __u32 n_size, void *buf, __u32 size) =
> +	(void *) BPF_FUNC_krsi_get_env_var;
>   static unsigned int (*bpf_get_socket_uid)(void *ctx) =
>   	(void *) BPF_FUNC_get_socket_uid;
>   static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> 

Powered by blists - more mailing lists