[PATCH bpf-next v2 0/2] clean-up for BPF_LOG_KERNEL log level

[PATCH bpf-next v2 0/2] clean-up for BPF_LOG_KERNEL log level

[Hou Tao]

Hi,

There are just two clean-up patches for BPF_LOG_KERNEL log level:
patch #1 fixes the possible extra newline for bpf_log() and removes
the unnecessary calculation and truncation, and patch #2 disallows
BPF_LOG_KERNEL log level for bpf_btf_load().

Comments are welcome.

Regards,
Tao

Change Log:
v2:
  * rebased on bpf-next
  * patch #1: add a trailing newline if needed (suggested by Martin)
  * add patch #2
 
v1: https://www.spinics.net/lists/bpf/msg48550.html

Hou Tao (2):
  bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level
  bpf: disallow BPF_LOG_KERNEL log level for sys(BPF_BTF_LOAD)

 include/linux/bpf_verifier.h |  6 ++++++
 kernel/bpf/btf.c             |  3 +--
 kernel/bpf/verifier.c        | 16 +++++++++-------
 3 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.29.2

[PATCH bpf-next v2 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level

[Hou Tao]

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@huawei.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");
 		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
-- 
2.29.2

Re: [PATCH bpf-next v2 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level

[Daniel Borkmann]

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@huawei.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
> 

Re: [PATCH bpf-next v2 1/2] bpf: clean-up bpf_verifier_vlog() for BPF_LOG_KERNEL log level

[Hou Tao]

Hi,

On 11/2/2021 6:01 AM, Daniel Borkmann wrote:
> 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
>>
>>       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?
>
My bad, and will do it in v3.

[PATCH bpf-next v2 2/2] bpf: disallow BPF_LOG_KERNEL log level for sys(BPF_BTF_LOAD)

[Hou Tao]

BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
to set log level as BPF_LOG_KERNEL. The same checking has already
been done in bpf_check(), so factor out a helper to check the
validity of log attributes and use it in both places.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 include/linux/bpf_verifier.h | 6 ++++++
 kernel/bpf/btf.c             | 3 +--
 kernel/bpf/verifier.c        | 6 +++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index c8a78e830fca..b36a0da8d5cf 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -396,6 +396,12 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
 		 log->level == BPF_LOG_KERNEL);
 }
 
+static inline bool bpf_verifier_log_attr_valid(const struct bpf_verifier_log *log)
+{
+	return (log->len_total >= 128 && log->len_total <= UINT_MAX >> 2 &&
+		log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK));
+}
+
 #define BPF_MAX_SUBPROGS 256
 
 struct bpf_subprog_info {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index dbc3ad07e21b..ea8874eaedac 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4460,8 +4460,7 @@ static struct btf *btf_parse(bpfptr_t btf_data, u32 btf_data_size,
 		log->len_total = log_size;
 
 		/* log attributes have to be sane */
-		if (log->len_total < 128 || log->len_total > UINT_MAX >> 8 ||
-		    !log->level || !log->ubuf) {
+		if (!bpf_verifier_log_attr_valid(log)) {
 			err = -EINVAL;
 			goto errout;
 		}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 22f0d2292c2c..47ad91cea7e9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -13935,11 +13935,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr)
 		log->ubuf = (char __user *) (unsigned long) attr->log_buf;
 		log->len_total = attr->log_size;
 
-		ret = -EINVAL;
 		/* log attributes have to be sane */
-		if (log->len_total < 128 || log->len_total > UINT_MAX >> 2 ||
-		    !log->level || !log->ubuf || log->level & ~BPF_LOG_MASK)
+		if (!bpf_verifier_log_attr_valid(log)) {
+			ret = -EINVAL;
 			goto err_unlock;
+		}
 	}
 
 	if (IS_ERR(btf_vmlinux)) {
-- 
2.29.2

Re: [PATCH bpf-next v2 2/2] bpf: disallow BPF_LOG_KERNEL log level for sys(BPF_BTF_LOAD)

[Daniel Borkmann]

On 10/29/21 3:53 PM, Hou Tao wrote:
> BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
> to set log level as BPF_LOG_KERNEL. The same checking has already
> been done in bpf_check(), so factor out a helper to check the
> validity of log attributes and use it in both places.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>   include/linux/bpf_verifier.h | 6 ++++++
>   kernel/bpf/btf.c             | 3 +--
>   kernel/bpf/verifier.c        | 6 +++---
>   3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index c8a78e830fca..b36a0da8d5cf 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -396,6 +396,12 @@ static inline bool bpf_verifier_log_needed(const struct bpf_verifier_log *log)
>   		 log->level == BPF_LOG_KERNEL);
>   }
>   
> +static inline bool bpf_verifier_log_attr_valid(const struct bpf_verifier_log *log)
> +{
> +	return (log->len_total >= 128 && log->len_total <= UINT_MAX >> 2 &&
> +		log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK));

nit: No surrounding () needed.

This should probably also get a Fixes tag wrt BPF_LOG_KERNEL exposure?

Is there a need to bump log->len_total for BTF so significantly?

> +}
> +
>   #define BPF_MAX_SUBPROGS 256
>   
>   struct bpf_subprog_info {
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index dbc3ad07e21b..ea8874eaedac 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4460,8 +4460,7 @@ static struct btf *btf_parse(bpfptr_t btf_data, u32 btf_data_size,
>   		log->len_total = log_size;
>   
>   		/* log attributes have to be sane */
> -		if (log->len_total < 128 || log->len_total > UINT_MAX >> 8 ||
> -		    !log->level || !log->ubuf) {
> +		if (!bpf_verifier_log_attr_valid(log)) {
>   			err = -EINVAL;
>   			goto errout;
>   		}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 22f0d2292c2c..47ad91cea7e9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13935,11 +13935,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr)
>   		log->ubuf = (char __user *) (unsigned long) attr->log_buf;
>   		log->len_total = attr->log_size;
>   
> -		ret = -EINVAL;
>   		/* log attributes have to be sane */
> -		if (log->len_total < 128 || log->len_total > UINT_MAX >> 2 ||
> -		    !log->level || !log->ubuf || log->level & ~BPF_LOG_MASK)
> +		if (!bpf_verifier_log_attr_valid(log)) {
> +			ret = -EINVAL;
>   			goto err_unlock;
> +		}
>   	}
>   
>   	if (IS_ERR(btf_vmlinux)) {
> 

Re: [PATCH bpf-next v2 2/2] bpf: disallow BPF_LOG_KERNEL log level for sys(BPF_BTF_LOAD)

[Hou Tao]

Hi,

On 11/2/2021 5:59 AM, Daniel Borkmann wrote:
> On 10/29/21 3:53 PM, Hou Tao wrote:
>> BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
>> to set log level as BPF_LOG_KERNEL. The same checking has already
>> been done in bpf_check(), so factor out a helper to check the
>> validity of log attributes and use it in both places.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>   include/linux/bpf_verifier.h | 6 ++++++
>>   kernel/bpf/btf.c             | 3 +--
>>   kernel/bpf/verifier.c        | 6 +++---
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index c8a78e830fca..b36a0da8d5cf 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -396,6 +396,12 @@ static inline bool bpf_verifier_log_needed(const struct
>> bpf_verifier_log *log)
>>            log->level == BPF_LOG_KERNEL);
>>   }
>>   +static inline bool bpf_verifier_log_attr_valid(const struct
>> bpf_verifier_log *log)
>> +{
>> +    return (log->len_total >= 128 && log->len_total <= UINT_MAX >> 2 &&
>> +        log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK));
>
> nit: No surrounding () needed.
Will fix.
>
> This should probably also get a Fixes tag wrt BPF_LOG_KERNEL exposure?
If log->level is set as BPF_LOG_KERNEL, the only harm is the user-space tool
(still need being bpf_capable()) may flood the kernel with BPF error message,
so i didn't add it. Adding the Fixes tags incurs no harm, so will do in v3.
>
> Is there a need to bump log->len_total for BTF so significantly?
>
I had noticed the values of these two max length are different, but doesn't find
any clue about why the different is necessary. So just use the bigger one for
the simplicity of bpf_verifier_log_attr_valid().  Will pass the required max
length to bpf_verifier_log_attr_valid() in v3.