We have a usecase where we want to audit symbol names (if available) in
callback registration hooks. (ex: fentry/nf_register_net_hook)
A few months back, I proposed a bpf_kallsyms_lookup series but it was
decided in the reviews that a more generic helper, bpf_snprintf, would
be more useful.
This series implements the helper according to the feedback received in
https://lore.kernel.org/bpf/[email protected]/T/#u
- A new arg type guarantees the NULL-termination of string arguments and
lets us pass format strings in only one arg
- A new helper is implemented using that guarantee. Because the format
string is known at verification time, the format string validation is
done by the verifier
- To implement a series of tests for bpf_snprintf, the logic for
marshalling variadic args in a fixed-size array is reworked as per:
https://lore.kernel.org/bpf/[email protected]/T/#u
---
Changes in v2:
- Extracted the format validation/argument sanitization in a generic way
for all printf-like helpers.
- bpf_snprintf's str_size can now be 0
- bpf_snprintf is now exposed to all BPF program types
- We now preempt_disable when using a per-cpu temporary buffer
- Addressed a few cosmetic changes
Florent Revest (6):
bpf: Factorize bpf_trace_printk and bpf_seq_printf
bpf: Add a ARG_PTR_TO_CONST_STR argument type
bpf: Add a bpf_snprintf helper
libbpf: Initialize the bpf_seq_printf parameters array field by field
libbpf: Introduce a BPF_SNPRINTF helper macro
selftests/bpf: Add a series of tests for bpf_snprintf
include/linux/bpf.h | 7 +
include/uapi/linux/bpf.h | 28 +
kernel/bpf/helpers.c | 2 +
kernel/bpf/verifier.c | 79 +++
kernel/trace/bpf_trace.c | 581 +++++++++---------
tools/include/uapi/linux/bpf.h | 28 +
tools/lib/bpf/bpf_tracing.h | 44 +-
.../selftests/bpf/prog_tests/snprintf.c | 65 ++
.../selftests/bpf/progs/test_snprintf.c | 59 ++
9 files changed, 604 insertions(+), 289 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
--
2.31.0.291.g576ba9dcdaf-goog
This type provides the guarantee that an argument is going to be a const
pointer to somewhere in a read-only map value. It also checks that this
pointer is followed by a zero character before the end of the map value.
Signed-off-by: Florent Revest <[email protected]>
---
include/linux/bpf.h | 1 +
kernel/bpf/verifier.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a25730eaa148..7b5319d75b3e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,6 +308,7 @@ enum bpf_arg_type {
ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */
ARG_PTR_TO_FUNC, /* pointer to a bpf program function */
ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */
+ ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */
__BPF_ARG_TYPE_MAX,
};
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e26c5170c953..9e03608725b4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU
static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
+static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
[ARG_PTR_TO_MAP_KEY] = &map_key_value_types,
@@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
[ARG_PTR_TO_PERCPU_BTF_ID] = &percpu_btf_ptr_types,
[ARG_PTR_TO_FUNC] = &func_ptr_types,
[ARG_PTR_TO_STACK_OR_NULL] = &stack_ptr_types,
+ [ARG_PTR_TO_CONST_STR] = &const_str_ptr_types,
};
static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
@@ -4881,6 +4883,42 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
if (err)
return err;
err = check_ptr_alignment(env, reg, 0, size, true);
+ } else if (arg_type == ARG_PTR_TO_CONST_STR) {
+ struct bpf_map *map = reg->map_ptr;
+ int map_off;
+ u64 map_addr;
+ char *str_ptr;
+
+ if (reg->type != PTR_TO_MAP_VALUE || !map ||
+ !bpf_map_is_rdonly(map)) {
+ verbose(env, "R%d does not point to a readonly map'\n", regno);
+ return -EACCES;
+ }
+
+ if (!tnum_is_const(reg->var_off)) {
+ verbose(env, "R%d is not a constant address'\n", regno);
+ return -EACCES;
+ }
+
+ if (!map->ops->map_direct_value_addr) {
+ verbose(env, "no direct value access support for this map type\n");
+ return -EACCES;
+ }
+
+ err = check_map_access(env, regno, reg->off,
+ map->value_size - reg->off, false);
+ if (err)
+ return err;
+
+ map_off = reg->off + reg->var_off.value;
+ err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
+ if (err)
+ return err;
+
+ str_ptr = (char *)(long)(map_addr);
+ if (!strnchr(str_ptr + map_off,
+ map->value_size - reg->off - map_off, 0))
+ verbose(env, "string is not zero-terminated\n");
}
return err;
--
2.31.0.291.g576ba9dcdaf-goog
When initializing the __param array with a one liner, if all args are
const, the initial array value will be placed in the rodata section but
because libbpf does not support relocation in the rodata section, any
pointer in this array will stay NULL.
Fixes: c09add2fbc5a ("tools/libbpf: Add bpf_iter support")
Signed-off-by: Florent Revest <[email protected]>
---
tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index f9ef37707888..d9a4c3f77ff4 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -413,6 +413,22 @@ typeof(name(0)) name(struct pt_regs *ctx) \
} \
static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
+#define ___bpf_fill0(arr, p, x)
+#define ___bpf_fill1(arr, p, x) arr[p] = x
+#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
+#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
+#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
+#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
+#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
+#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
+#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
+#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
+#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
+#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
+#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
+#define ___bpf_fill(arr, args...) \
+ ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
+
/*
* BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
* in a structure.
@@ -421,12 +437,14 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
({ \
_Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ unsigned long long ___param[___bpf_narg(args)]; \
static const char ___fmt[] = fmt; \
- unsigned long long ___param[] = { args }; \
+ int __ret; \
+ ___bpf_fill(___param, args); \
_Pragma("GCC diagnostic pop") \
- int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
- ___param, sizeof(___param)); \
- ___ret; \
+ __ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
+ ___param, sizeof(___param)); \
+ __ret; \
})
#endif
--
2.31.0.291.g576ba9dcdaf-goog
On Tue, Mar 23, 2021 at 7:23 PM Florent Revest <[email protected]> wrote:
>
> This type provides the guarantee that an argument is going to be a const
> pointer to somewhere in a read-only map value. It also checks that this
> pointer is followed by a zero character before the end of the map value.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> include/linux/bpf.h | 1 +
> kernel/bpf/verifier.c | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a25730eaa148..7b5319d75b3e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -308,6 +308,7 @@ enum bpf_arg_type {
> ARG_PTR_TO_PERCPU_BTF_ID, /* pointer to in-kernel percpu type */
> ARG_PTR_TO_FUNC, /* pointer to a bpf program function */
> ARG_PTR_TO_STACK_OR_NULL, /* pointer to stack or NULL */
> + ARG_PTR_TO_CONST_STR, /* pointer to a null terminated read-only string */
> __BPF_ARG_TYPE_MAX,
> };
>
[...]
> +
> + map_off = reg->off + reg->var_off.value;
> + err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> + if (err)
> + return err;
> +
> + str_ptr = (char *)(long)(map_addr);
> + if (!strnchr(str_ptr + map_off,
> + map->value_size - reg->off - map_off, 0))
you are double subtracting reg->off here. isn't map->value_size -
map_off what you want?
> + verbose(env, "string is not zero-terminated\n");
I'd prefer `return -EINVAL;`, but at least set err, otherwise what's the point?
> }
>
> return err;
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
On Tue, Mar 23, 2021 at 7:23 PM Florent Revest <[email protected]> wrote:
>
> When initializing the __param array with a one liner, if all args are
> const, the initial array value will be placed in the rodata section but
> because libbpf does not support relocation in the rodata section, any
> pointer in this array will stay NULL.
>
> Fixes: c09add2fbc5a ("tools/libbpf: Add bpf_iter support")
> Signed-off-by: Florent Revest <[email protected]>
> ---
> tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index f9ef37707888..d9a4c3f77ff4 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -413,6 +413,22 @@ typeof(name(0)) name(struct pt_regs *ctx) \
> } \
> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> +#define ___bpf_fill0(arr, p, x)
can you please double-check that no-argument BPF_SEQ_PRINTF won't
generate a warning about spurious ';'? Maybe it's better to have zero
case as `do {} while(0);` ?
> +#define ___bpf_fill1(arr, p, x) arr[p] = x
> +#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
> +#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
> +#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
> +#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
> +#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
> +#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
> +#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
> +#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
> +#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
> +#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
> +#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
> +#define ___bpf_fill(arr, args...) \
> + ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
cool. this is regular enough to easily comprehend :)
> +
> /*
> * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
> * in a structure.
> @@ -421,12 +437,14 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> ({ \
> _Pragma("GCC diagnostic push") \
> _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + unsigned long long ___param[___bpf_narg(args)]; \
> static const char ___fmt[] = fmt; \
> - unsigned long long ___param[] = { args }; \
> + int __ret; \
> + ___bpf_fill(___param, args); \
> _Pragma("GCC diagnostic pop") \
Let's clean this up a little bit;
1. static const char ___fmt should be the very first
2. _Pragma scope should be minimal necessary, which includes only
___bpf_fill, right?
3. Empty line after int __ret; and let's keep three underscores for consistency.
> - int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> - ___param, sizeof(___param)); \
> - ___ret; \
> + __ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> + ___param, sizeof(___param)); \
> + __ret; \
but actually you don't need __ret at all, just bpf_seq_printf() here, right?
> })
>
> #endif
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
On Sat, Mar 27, 2021 at 12:01 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Tue, Mar 23, 2021 at 7:23 PM Florent Revest <[email protected]> wrote:
> >
> > When initializing the __param array with a one liner, if all args are
> > const, the initial array value will be placed in the rodata section but
> > because libbpf does not support relocation in the rodata section, any
> > pointer in this array will stay NULL.
> >
> > Fixes: c09add2fbc5a ("tools/libbpf: Add bpf_iter support")
> > Signed-off-by: Florent Revest <[email protected]>
> > ---
> > tools/lib/bpf/bpf_tracing.h | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > index f9ef37707888..d9a4c3f77ff4 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -413,6 +413,22 @@ typeof(name(0)) name(struct pt_regs *ctx) \
> > } \
> > static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> >
> > +#define ___bpf_fill0(arr, p, x)
>
> can you please double-check that no-argument BPF_SEQ_PRINTF won't
> generate a warning about spurious ';'? Maybe it's better to have zero
> case as `do {} while(0);` ?
>
> > +#define ___bpf_fill1(arr, p, x) arr[p] = x
> > +#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
> > +#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
> > +#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
> > +#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
> > +#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
> > +#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
> > +#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
> > +#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
> > +#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
> > +#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
> > +#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
> > +#define ___bpf_fill(arr, args...) \
> > + ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
>
> cool. this is regular enough to easily comprehend :)
>
> > +
> > /*
> > * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
> > * in a structure.
> > @@ -421,12 +437,14 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> > ({ \
> > _Pragma("GCC diagnostic push") \
> > _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> > + unsigned long long ___param[___bpf_narg(args)]; \
> > static const char ___fmt[] = fmt; \
> > - unsigned long long ___param[] = { args }; \
> > + int __ret; \
> > + ___bpf_fill(___param, args); \
> > _Pragma("GCC diagnostic pop") \
>
> Let's clean this up a little bit;
> 1. static const char ___fmt should be the very first
> 2. _Pragma scope should be minimal necessary, which includes only
> ___bpf_fill, right?
> 3. Empty line after int __ret; and let's keep three underscores for consistency.
>
>
> > - int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > - ___param, sizeof(___param)); \
> > - ___ret; \
> > + __ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
> > + ___param, sizeof(___param)); \
> > + __ret; \
>
> but actually you don't need __ret at all, just bpf_seq_printf() here, right?
Agreed with everything and also the indentation comment in 5/6, thanks.
On Fri, Mar 26, 2021 at 11:23 PM Andrii Nakryiko
<[email protected]> wrote:
> On Tue, Mar 23, 2021 at 7:23 PM Florent Revest <[email protected]> wrote:
> > +
> > + map_off = reg->off + reg->var_off.value;
> > + err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> > + if (err)
> > + return err;
> > +
> > + str_ptr = (char *)(long)(map_addr);
> > + if (!strnchr(str_ptr + map_off,
> > + map->value_size - reg->off - map_off, 0))
>
> you are double subtracting reg->off here. isn't map->value_size -
> map_off what you want?
Good catch!
> > + verbose(env, "string is not zero-terminated\n");
>
> I'd prefer `return -EINVAL;`, but at least set err, otherwise what's the point?
Ah yeah, absolutely.