netdev - Re: [PATCH] samples: bpf: fix: error handling regarding kprobe

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] [day] [month] [year] [list]

Date:   Thu, 22 Nov 2018 12:09:01 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     "Daniel T. Lee" <danieltimlee@...il.com>,
        Alexei Starovoitov <ast@...nel.org>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] samples: bpf: fix: error handling regarding kprobe_events

On 11/21/2018 05:07 PM, Daniel T. Lee wrote:
> Currently, kprobe_events failure won't be handled properly.
> Due to calling system() indirectly to write to kprobe_events,
> it can't be identified whether an error is derived from kprobe or system.
> 
>     // buf = "echo '%c:%s %s' >> /s/k/d/t/kprobe_events"
>     err = system(buf);
>     if (err < 0) {
>         printf("failed to create kprobe ..");
>         return -1;
>     }
> 
> For example, running ./tracex7 sample in ext4 partition,
> "echo p:open_ctree open_ctree >> /s/k/d/t/kprobe_events"
> gets 256 error code system() failure.
> => The error comes from kprobe, but it's not handled correctly.
> 
> According to man of system(3), it's return value
> just passes the termination status of the child shell
> rather than treating the error as -1. (don't care success)
> 
> Which means, currently it's not working as desired.
> (According to the upper code snippet)
> 
>     ex) running ./tracex7 with ext4 env.
>     # Current Output
>     sh: echo: I/O error
>     failed to open event open_ctree
> 
>     # Desired Output
>     failed to create kprobe 'open_ctree' error 'No such file or directory'
> 
> The problem is, error can't be verified whether from child ps or system.
> 
> But using write() directly can verify the command failure,
> and it will treat all error as -1.
> 
> So I suggest using write() directly to 'kprobe_events'
> rather than calling system().
> 
> Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>

Looks reasonable to me, thanks for the patch! One tiny nit below:

> ---
>  samples/bpf/bpf_load.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
> index e6d7e0fe155b..3e34d733f5f8 100644
> --- a/samples/bpf/bpf_load.c
> +++ b/samples/bpf/bpf_load.c
> @@ -54,6 +54,23 @@ static int populate_prog_array(const char *event, int prog_fd)
>  	return 0;
>  }
>  
> +static int write_kprobe_events(const char *val)
> +{
> +	int ret, flags;
> +
> +	if ((val != NULL) && (val[0] == '\0'))
> +		flags = O_WRONLY | O_TRUNC;
> +	else
> +		flags = O_WRONLY | O_APPEND;
> +
> +	int fd = open("/sys/kernel/debug/tracing/kprobe_events", flags);

Nit: could you move fd declaration to above where you have ret and
flags instead of mixing with the code in order to match kernel style?

> +	ret = write(fd, val, strlen(val));
> +	close(fd);
> +
> +	return ret;
> +}
> +
>  static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>  {
>  	bool is_socket = strncmp(event, "socket", 6) == 0;
> @@ -165,10 +182,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>  
>  #ifdef __x86_64__
>  		if (strncmp(event, "sys_", 4) == 0) {
> -			snprintf(buf, sizeof(buf),
> -				 "echo '%c:__x64_%s __x64_%s' >> /sys/kernel/debug/tracing/kprobe_events",
> -				 is_kprobe ? 'p' : 'r', event, event);
> -			err = system(buf);
> +			snprintf(buf, sizeof(buf), "%c:__x64_%s __x64_%s",
> +				is_kprobe ? 'p' : 'r', event, event);
> +			err = write_kprobe_events(buf);
>  			if (err >= 0) {
>  				need_normal_check = false;
>  				event_prefix = "__x64_";
> @@ -176,10 +192,9 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
>  		}
>  #endif
>  		if (need_normal_check) {
> -			snprintf(buf, sizeof(buf),
> -				 "echo '%c:%s %s' >> /sys/kernel/debug/tracing/kprobe_events",
> -				 is_kprobe ? 'p' : 'r', event, event);
> -			err = system(buf);
> +			snprintf(buf, sizeof(buf), "%c:%s %s",
> +				is_kprobe ? 'p' : 'r', event, event);
> +			err = write_kprobe_events(buf);
>  			if (err < 0) {
>  				printf("failed to create kprobe '%s' error '%s'\n",
>  				       event, strerror(errno));
> @@ -519,7 +534,7 @@ static int do_load_bpf_file(const char *path, fixup_map_cb fixup_map)
>  		return 1;
>  
>  	/* clear all kprobes */
> -	i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
> +	i = write_kprobe_events("");
>  
>  	/* scan over all elf sections to get license and map info */
>  	for (i = 1; i < ehdr.e_shnum; i++) {
> 

Thanks,
Daniel