Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
On Fri, 28 Sep 2018 at 06:38, Daniel Borkmann wrote:
>
> On 09/28/2018 01:26 AM, Joe Stringer wrote:
> > Teach the verifier a little bit about a new type of pointer, a
> > PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> > 'struct bpf_sock' structure.
> >
> > Signed-off-by: Joe Stringer
> [...]
> > +/* Return true if it's OK to have the same insn return a different type. */
> > +static bool reg_type_mismatch_ok(enum bpf_reg_type type)
> > +{
> > + switch (type) {
> > + case PTR_TO_CTX:
> > + case PTR_TO_SOCKET:
> > + case PTR_TO_SOCKET_OR_NULL:
> > + return false;
> > + default:
> > + return true;
> > + }
> > +}
> > +
> > +/* If an instruction was previously used with particular pointer types,
> > then we
> > + * need to be careful to avoid cases such as the below, where it may be ok
> > + * for one branch accessing the pointer, but not ok for the other branch:
> > + *
> > + * R1 = sock_ptr
> > + * goto X;
> > + * ...
> > + * R1 = some_other_valid_ptr;
> > + * goto X;
> > + * ...
> > + * R2 = *(u32 *)(R1 + 0);
> > + */
> > +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type
> > prev)
> > +{
> > + return src != prev && (!reg_type_mismatch_ok(src) ||
> > +!reg_type_mismatch_ok(prev));
> > +}
> > +
> > static int do_check(struct bpf_verifier_env *env)
> > {
> > struct bpf_verifier_state *state;
> > @@ -4812,9 +4894,7 @@ static int do_check(struct bpf_verifier_env *env)
> >*/
> > *prev_src_type = src_reg_type;
> >
> > - } else if (src_reg_type != *prev_src_type &&
> > -(src_reg_type == PTR_TO_CTX ||
> > - *prev_src_type == PTR_TO_CTX)) {
> > + } else if (reg_type_mismatch(src_reg_type,
> > *prev_src_type)) {
> > /* ABuser program is trying to use the same
> > insn
> >* dst_reg = *(u32*) (src_reg + off)
> >* with different pointer types:
> > @@ -4859,9 +4939,7 @@ static int do_check(struct bpf_verifier_env *env)
> >
> > if (*prev_dst_type == NOT_INIT) {
> > *prev_dst_type = dst_reg_type;
> > - } else if (dst_reg_type != *prev_dst_type &&
> > -(dst_reg_type == PTR_TO_CTX ||
> > - *prev_dst_type == PTR_TO_CTX)) {
> > + } else if (reg_type_mismatch(dst_reg_type,
> > *prev_dst_type)) {
> > verbose(env, "same insn cannot be used with
> > different pointers\n");
> > return -EINVAL;
>
> Can also be as follow-up later on, but it would be crucial to also have
> test_verifier tests on this logic here with mixing these pointer types
> from different branches (right now we only cover ctx there).
Thanks for the feedback. I've applied all of your suggestions.
Regarding these newer tests, I have added a few and will post that
with my next revision. Fortunately with the reference tracking it's
actually quite difficult to mix up the pointer types between socket
and another type, because if the type of the register is ambiguous
then you either end up leaking a reference or attempting to release
using a pointer to a non-socket. I've added tests for both of those
cases, along with attempts to read and write into offsets inside
ambiguous pointers which triggers most of these paths.
Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
On 09/28/2018 01:26 AM, Joe Stringer wrote:
> Teach the verifier a little bit about a new type of pointer, a
> PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> 'struct bpf_sock' structure.
>
> Signed-off-by: Joe Stringer
[...]
> +/* Return true if it's OK to have the same insn return a different type. */
> +static bool reg_type_mismatch_ok(enum bpf_reg_type type)
> +{
> + switch (type) {
> + case PTR_TO_CTX:
> + case PTR_TO_SOCKET:
> + case PTR_TO_SOCKET_OR_NULL:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +/* If an instruction was previously used with particular pointer types, then
> we
> + * need to be careful to avoid cases such as the below, where it may be ok
> + * for one branch accessing the pointer, but not ok for the other branch:
> + *
> + * R1 = sock_ptr
> + * goto X;
> + * ...
> + * R1 = some_other_valid_ptr;
> + * goto X;
> + * ...
> + * R2 = *(u32 *)(R1 + 0);
> + */
> +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type prev)
> +{
> + return src != prev && (!reg_type_mismatch_ok(src) ||
> +!reg_type_mismatch_ok(prev));
> +}
> +
> static int do_check(struct bpf_verifier_env *env)
> {
> struct bpf_verifier_state *state;
> @@ -4812,9 +4894,7 @@ static int do_check(struct bpf_verifier_env *env)
>*/
> *prev_src_type = src_reg_type;
>
> - } else if (src_reg_type != *prev_src_type &&
> -(src_reg_type == PTR_TO_CTX ||
> - *prev_src_type == PTR_TO_CTX)) {
> + } else if (reg_type_mismatch(src_reg_type,
> *prev_src_type)) {
> /* ABuser program is trying to use the same insn
>* dst_reg = *(u32*) (src_reg + off)
>* with different pointer types:
> @@ -4859,9 +4939,7 @@ static int do_check(struct bpf_verifier_env *env)
>
> if (*prev_dst_type == NOT_INIT) {
> *prev_dst_type = dst_reg_type;
> - } else if (dst_reg_type != *prev_dst_type &&
> -(dst_reg_type == PTR_TO_CTX ||
> - *prev_dst_type == PTR_TO_CTX)) {
> + } else if (reg_type_mismatch(dst_reg_type,
> *prev_dst_type)) {
> verbose(env, "same insn cannot be used with
> different pointers\n");
> return -EINVAL;
Can also be as follow-up later on, but it would be crucial to also have
test_verifier tests on this logic here with mixing these pointer types
from different branches (right now we only cover ctx there).
Thanks,
Daniel
Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
On 09/28/2018 01:26 AM, Joe Stringer wrote:
> Teach the verifier a little bit about a new type of pointer, a
> PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> 'struct bpf_sock' structure.
>
> Signed-off-by: Joe Stringer
[...]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 72db8afb7cb6..057af3dc9f08 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5394,23 +5394,29 @@ static bool __sock_filter_check_size(int off, int
> size,
> return size == size_default;
> }
>
> -static bool sock_filter_is_valid_access(int off, int size,
> - enum bpf_access_type type,
> - const struct bpf_prog *prog,
> - struct bpf_insn_access_aux *info)
> +bool bpf_sock_is_valid_access(int off, int size, enum bpf_access_type type,
> + struct bpf_insn_access_aux *info)
> {
> if (off < 0 || off >= sizeof(struct bpf_sock))
> return false;
> if (off % size != 0)
> return false;
> - if (!__sock_filter_check_attach_type(off, type,
> - prog->expected_attach_type))
> - return false;
> if (!__sock_filter_check_size(off, size, info))
> return false;
> return true;
> }
>
> +static bool sock_filter_is_valid_access(int off, int size,
> + enum bpf_access_type type,
> + const struct bpf_prog *prog,
> + struct bpf_insn_access_aux *info)
> +{
> + if (!__sock_filter_check_attach_type(off, type,
> + prog->expected_attach_type))
> + return false;
> + return bpf_sock_is_valid_access(off, size, type, info);
> +}
This one here should also be swapped to make it more robust, meaning the
__sock_filter_check_attach_type() should come in a second step after basic
sanity checks have been completed, not before them. E.g. out of bounds read
access would then indicate a "good" access in the first one.
Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
Hi Joe,
On 09/28/2018 01:26 AM, Joe Stringer wrote:
> Teach the verifier a little bit about a new type of pointer, a
> PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> 'struct bpf_sock' structure.
>
> Signed-off-by: Joe Stringer
[...]
> }
> @@ -1726,6 +1755,14 @@ static int check_mem_access(struct bpf_verifier_env
> *env, int insn_idx, u32 regn
> err = check_flow_keys_access(env, off, size);
> if (!err && t == BPF_READ && value_regno >= 0)
> mark_reg_unknown(env, regs, value_regno);
> + } else if (reg->type == PTR_TO_SOCKET) {
> + if (t == BPF_WRITE) {
> + verbose(env, "cannot write into socket\n");
> + return -EACCES;
> + }
> + err = check_sock_access(env, regno, off, size, t);
> + if (!err && value_regno >= 0)
> + mark_reg_unknown(env, regs, value_regno);
Not an issue today, but this is quite fragile and very easy to miss, if we
allow to enable writes into ptr_to_socket in future e.g. mark or others,
then lifting above will not be enough. E.g. see check_xadd() and friends,
this rejects writes to ctx via f37a8cb84cce ("bpf: reject stores into ctx
via st and xadd") as otherwise this would bypass the context rewriter. So
I think we should add PTR_TO_SOCKET to is_ctx_reg() as well to have a full
guarantee this won't happen.
> } else {
> verbose(env, "R%d invalid mem access '%s'\n", regno,
> reg_type_str[reg->type]);
Thanks,
Daniel
Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
On Thu, Sep 27, 2018 at 04:26:51PM -0700, Joe Stringer wrote: > Teach the verifier a little bit about a new type of pointer, a > PTR_TO_SOCKET. This pointer type is accessed from BPF through the > 'struct bpf_sock' structure. > > Signed-off-by: Joe Stringer Acked-by: Alexei Starovoitov
[PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type
Teach the verifier a little bit about a new type of pointer, a
PTR_TO_SOCKET. This pointer type is accessed from BPF through the
'struct bpf_sock' structure.
Signed-off-by: Joe Stringer
---
v2: Reuse reg_type_mismatch() in more places
Reduce the number of passes at convert_ctx_access()
v3: Fix build with !CONFIG_NET
---
include/linux/bpf.h | 34 ++
include/linux/bpf_verifier.h | 2 +
kernel/bpf/verifier.c| 120 +++
net/core/filter.c| 30 +
4 files changed, 160 insertions(+), 26 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 018299a595c8..027697b6a22f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -154,6 +154,7 @@ enum bpf_arg_type {
ARG_PTR_TO_CTX, /* pointer to context */
ARG_ANYTHING, /* any (initialized) argument is ok */
+ ARG_PTR_TO_SOCKET, /* pointer to bpf_sock */
};
/* type of values returned from helper functions */
@@ -162,6 +163,7 @@ enum bpf_return_type {
RET_VOID, /* function doesn't return anything */
RET_PTR_TO_MAP_VALUE, /* returns a pointer to map elem value
*/
RET_PTR_TO_MAP_VALUE_OR_NULL, /* returns a pointer to map elem value
or NULL */
+ RET_PTR_TO_SOCKET_OR_NULL, /* returns a pointer to a socket or
NULL */
};
/* eBPF function prototype used by verifier to allow BPF_CALLs from eBPF
programs
@@ -213,6 +215,8 @@ enum bpf_reg_type {
PTR_TO_PACKET, /* reg points to skb->data */
PTR_TO_PACKET_END, /* skb->data + headlen */
PTR_TO_FLOW_KEYS,/* reg points to bpf_flow_keys */
+ PTR_TO_SOCKET, /* reg points to struct bpf_sock */
+ PTR_TO_SOCKET_OR_NULL, /* reg points to struct bpf_sock or NULL */
};
/* The information passed from prog-specific *_is_valid_access
@@ -343,6 +347,11 @@ const struct bpf_func_proto
*bpf_get_trace_printk_proto(void);
typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src,
unsigned long off, unsigned long len);
+typedef u32 (*bpf_convert_ctx_access_t)(enum bpf_access_type type,
+ const struct bpf_insn *src,
+ struct bpf_insn *dst,
+ struct bpf_prog *prog,
+ u32 *target_size);
u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy);
@@ -836,4 +845,29 @@ extern const struct bpf_func_proto
bpf_get_local_storage_proto;
void bpf_user_rnd_init_once(void);
u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
+#if defined(CONFIG_NET)
+bool bpf_sock_is_valid_access(int off, int size, enum bpf_access_type type,
+ struct bpf_insn_access_aux *info);
+u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog,
+ u32 *target_size);
+#else
+static inline bool bpf_sock_is_valid_access(int off, int size,
+ enum bpf_access_type type,
+ struct bpf_insn_access_aux *info)
+{
+ return false;
+}
+static inline u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
+ const struct bpf_insn *si,
+ struct bpf_insn *insn_buf,
+ struct bpf_prog *prog,
+ u32 *target_size)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_BPF_H */
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index af262b97f586..23a2b17bfd75 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -58,6 +58,8 @@ struct bpf_reg_state {
* offset, so they can share range knowledge.
* For PTR_TO_MAP_VALUE_OR_NULL this is used to share which map value we
* came from, when one is tested for != NULL.
+* For PTR_TO_SOCKET this is used to share which pointers retain the
+* same reference to the socket, to determine proper reference freeing.
*/
u32 id;
/* For scalar types (SCALAR_VALUE), this represents our knowledge of
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index bbb0a812ee81..d4abbf0d5727 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -80,8 +80,8 @@ static const struct bpf_verifier_ops * const
bpf_verifier_ops[] = {
* (like pointer plus pointer becomes SCALAR_VALUE type)
*
* When verifier sees load or store instructions the type of base
