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  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]
Date:   Thu, 18 Jun 2020 12:09:35 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Andrii Nakryiko <andriin@...com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, ast@...com, daniel@...earbox.net
Cc:     andrii.nakryiko@...il.com, kernel-team@...com,
        Andrii Nakryiko <andriin@...com>,
        Christoph Hellwig <hch@....de>
Subject: RE: [PATCH bpf 2/2] selftests/bpf: add variable-length data
 concatenation pattern test

Andrii Nakryiko wrote:
> Add selftest that validates variable-length data reading and concatentation
> with one big shared data array. This is a common pattern in production use for
> monitoring and tracing applications, that potentially can read a lot of data,
> but usually reads much less. Such pattern allows to determine precisely what
> amount of data needs to be sent over perfbuf/ringbuf and maximize efficiency.
> 
> This is the first BPF selftest that at all looks at and tests
> bpf_probe_read_str()-like helper's return value, closing a major gap in BPF
> testing. It surfaced the problem with bpf_probe_read_kernel_str() returning
> 0 on success, instead of amount of bytes successfully read.
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---

[...]

> +/* .data */
> +int payload2_len1 = -1;
> +int payload2_len2 = -1;
> +int total2 = -1;
> +char payload2[MAX_LEN + MAX_LEN] = { 1 };
> +
> +SEC("raw_tp/sys_enter")
> +int handler64(void *regs)
> +{
> +	int pid = bpf_get_current_pid_tgid() >> 32;
> +	void *payload = payload1;
> +	u64 len;
> +
> +	/* ignore irrelevant invocations */
> +	if (test_pid != pid || !capture)
> +		return 0;
> +
> +	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
> +	if (len <= MAX_LEN) {

Took me a bit grok this. You are relying on the fact that in errors,
such as a page fault, will encode to a large u64 value and so you
verifier is happy. But most of my programs actually want to distinguish
between legitimate errors on the probe vs buffer overrun cases.

Can we make these tests do explicit check for errors. For example,

  if (len < 0) goto abort;

But this also breaks your types here. This is what I was trying to
point out in the 1/2 patch thread. Wanted to make the point here as
well in case it wasn't clear. Not sure I did the best job explaining.

> +		payload += len;
> +		payload1_len1 = len;
> +	}
> +
> +	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
> +	if (len <= MAX_LEN) {
> +		payload += len;
> +		payload1_len2 = len;
> +	}
> +
> +	total1 = payload - (void *)payload1;
> +
> +	return 0;
> +}

Powered by blists - more mailing lists