| Message ID | 20211029135321.94065-3-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | clean-up for BPF_LOG_KERNEL log level | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: songliubraving@fb.com john.fastabend@gmail.com kpsingh@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 40 this patch: 40 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 40 this patch: 40 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next | fail | VM_Test |
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)) { >
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.
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)) {
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(-)