netdev - Re: [PATCH bpf-next v2 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG

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: <ebdd6730-1dfc-1889-eae9-00211bd82803@iogearbox.net>
Date:   Mon, 1 Nov 2021 23:01:30 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Hou Tao <houtao1@...wei.com>, Alexei Starovoitov <ast@...nel.org>
Cc:     Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 1/2] bpf: clean-up bpf_verifier_vlog() for
 BPF_LOG_KERNEL log level

On 10/29/21 3:53 PM, Hou Tao wrote:
> An extra newline will output for bpf_log() with BPF_LOG_KERNEL level
> as shown below:
> 
> [   52.095704] BPF:The function test_3 has 12 arguments. Too many.
> [   52.095704]
> [   52.096896] Error in parsing func ptr test_3 in struct bpf_dummy_ops
> 
> Now all bpf_log() are ended by newline, but not all btf_verifier_log()
> are ended by newline, so checking whether or not the log message
> has the trailing newline and adding a newline if not.
> 
> Also there is no need to calculate the left userspace buffer size
> for kernel log output and to truncate the output by '\0' which
> has already been done by vscnprintf(), so only do these for
> userspace log output.
> 
> Signed-off-by: Hou Tao <houtao1@...wei.com>
> ---
>   kernel/bpf/verifier.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 3c8aa7df1773..22f0d2292c2c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -299,13 +299,15 @@ void bpf_verifier_vlog(struct bpf_verifier_log *log, const char *fmt,
>   	WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
>   		  "verifier log line truncated - local buffer too short\n");
>   
> -	n = min(log->len_total - log->len_used - 1, n);
> -	log->kbuf[n] = '\0';
> -
>   	if (log->level == BPF_LOG_KERNEL) {
> -		pr_err("BPF:%s\n", log->kbuf);
> +		bool newline = n > 0 && log->kbuf[n - 1] == '\n';
> +
> +		pr_err("BPF:%s%s", log->kbuf, newline ? "" : "\n");

nit: Given you change this anyway, is there a reason not to go with "BPF: %s%s" instead?

>   		return;
>   	}
> +
> +	n = min(log->len_total - log->len_used - 1, n);
> +	log->kbuf[n] = '\0';
>   	if (!copy_to_user(log->ubuf + log->len_used, log->kbuf, n + 1))
>   		log->len_used += n;
>   	else
>