Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Yonghong Song <yhs@fb.com>
To: KP Singh <kpsingh@chromium.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>
Cc: "Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"James Morris" <jmorris@namei.org>,
	"Kees Cook" <keescook@chromium.org>,
	"Thomas Garnier" <thgarnie@chromium.org>,
	"Michael Halcrow" <mhalcrow@google.com>,
	"Paul Turner" <pjt@google.com>,
	"Brendan Gregg" <brendan.d.gregg@gmail.com>,
	"Jann Horn" <jannh@google.com>,
	"Matthew Garrett" <mjg59@google.com>,
	"Christian Brauner" <christian@brauner.io>,
	"Mickaël Salaün" <mic@digikod.net>,
	"Florent Revest" <revest@chromium.org>,
	"Martin Lau" <kafai@fb.com>, "Song Liu" <songliubraving@fb.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	"Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Nicolas Ferre" <nicolas.ferre@microchip.com>,
	"Stanislav Fomichev" <sdf@google.com>,
	"Quentin Monnet" <quentin.monnet@netronome.com>,
	"Andrey Ignatov" <rdna@fb.com>, "Joe Stringer" <joe@wand.net.nz>
Subject: Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable
Date: Sun, 15 Sep 2019 00:16:48 +0000
Message-ID: <0a5386c9-3dbd-1ed8-d94c-d866c6369743@fb.com> (raw)
In-Reply-To: <20190910115527.5235-13-kpsingh@chromium.org>



On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.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@google.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) =
> 

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-10 11:55 [RFC v1 00/14] Kernel Runtime Security Instrumentation KP Singh
2019-09-10 11:55 ` [RFC v1 01/14] krsi: Add a skeleton and config options for the KRSI LSM KP Singh
2019-09-10 11:55 ` [RFC v1 02/14] krsi: Introduce types for KRSI eBPF KP Singh
2019-09-10 11:55 ` [RFC v1 03/14] bpf: krsi: sync BPF UAPI header with tools KP Singh
2019-09-10 11:55 ` [RFC v1 04/14] krsi: Add support in libbpf for BPF_PROG_TYPE_KRSI KP Singh
2019-09-14 16:09   ` Yonghong Song
2019-09-10 11:55 ` [RFC v1 05/14] krsi: Initialize KRSI hooks and create files in securityfs KP Singh
2019-09-14 16:26   ` Yonghong Song
2019-09-10 11:55 ` [RFC v1 06/14] krsi: Implement eBPF operations, attachment and execution KP Singh
2019-09-14 16:56   ` Yonghong Song
2019-09-15  0:37     ` Yonghong Song
2019-09-10 11:55 ` [RFC v1 07/14] krsi: Check for premissions on eBPF attachment KP Singh
2019-09-10 11:55 ` [RFC v1 08/14] krsi: Show attached program names in hook read handler KP Singh
2019-09-10 11:55 ` [RFC v1 09/14] krsi: Add a helper function for bpf_perf_event_output KP Singh
2019-09-14 18:23   ` Yonghong Song
2019-09-10 11:55 ` [RFC v1 10/14] krsi: Handle attachment of the same program KP Singh
2019-09-10 11:55 ` [RFC v1 11/14] krsi: Pin argument pages in bprm_check_security hook KP Singh
2019-09-10 11:55 ` [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable KP Singh
2019-09-15  0:16   ` Yonghong Song [this message]
2019-09-16 13:00     ` KP Singh
2019-09-17 16:58       ` Yonghong Song
2019-09-17 19:36         ` KP Singh
2019-09-10 11:55 ` [RFC v1 13/14] krsi: Provide an example to read and log environment variables KP Singh
2019-09-15  0:24   ` Yonghong Song
2019-09-10 11:55 ` [RFC v1 14/14] krsi: Pin arg pages only when needed KP Singh
2019-09-15  0:33   ` Yonghong Song
2019-09-15  1:40     ` KP Singh
2019-09-15 19:45       ` Yonghong Song

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0a5386c9-3dbd-1ed8-d94c-d866c6369743@fb.com \
    --to=yhs@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brendan.d.gregg@gmail.com \
    --cc=christian@brauner.io \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=jannh@google.com \
    --cc=jmorris@namei.org \
    --cc=joe@wand.net.nz \
    --cc=kafai@fb.com \
    --cc=keescook@chromium.org \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mchehab+samsung@kernel.org \
    --cc=mhalcrow@google.com \
    --cc=mic@digikod.net \
    --cc=mjg59@google.com \
    --cc=nicolas.ferre@microchip.com \
    --cc=pjt@google.com \
    --cc=quentin.monnet@netronome.com \
    --cc=rdna@fb.com \
    --cc=revest@chromium.org \
    --cc=sdf@google.com \
    --cc=serge@hallyn.com \
    --cc=songliubraving@fb.com \
    --cc=thgarnie@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox