netdev - Re: [PATCH bpf] bpf: Sanitize the bpf_struct

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]

Date:   Fri, 13 Mar 2020 17:16:55 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: Sanitize the bpf_struct_ops tcp-cc name

On Fri, Mar 13, 2020 at 4:37 PM Martin KaFai Lau <kafai@...com> wrote:
>
> The bpf_struct_ops tcp-cc name should be sanitized in order to
> avoid problematic chars (e.g. whitespaces).
>
> This patch reuses the bpf_obj_name_cpy() for accepting the same set
> of characters in order to keep a consistent bpf programming experience.
> A "size" param is added.  Also, the strlen is returned on success so
> that the caller (like the bpf_tcp_ca here) can error out on empty name.
> The existing callers of the bpf_obj_name_cpy() only need to change the
> testing statement to "if (err < 0)".  For all these existing callers,
> the err will be overwritten later, so no extra change is needed
> for the new strlen return value.
>
> Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---
>  include/linux/bpf.h   |  1 +
>  kernel/bpf/syscall.c  | 24 +++++++++++++-----------
>  net/ipv4/bpf_tcp_ca.c |  7 ++-----
>  3 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 49b1a70e12c8..212991f6f2a5 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -160,6 +160,7 @@ static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
>  }
>  void copy_map_value_locked(struct bpf_map *map, void *dst, void *src,
>                            bool lock_src);
> +int bpf_obj_name_cpy(char *dst, const char *src, unsigned int size);
>
>  struct bpf_offload_dev;
>  struct bpf_offloaded_map;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0c7fb0d4836d..d2984bf362c2 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -696,14 +696,14 @@ int bpf_get_file_flag(int flags)
>                    offsetof(union bpf_attr, CMD##_LAST_FIELD) - \
>                    sizeof(attr->CMD##_LAST_FIELD)) != NULL
>
> -/* dst and src must have at least BPF_OBJ_NAME_LEN number of bytes.
> - * Return 0 on success and < 0 on error.
> +/* dst and src must have at least "size" number of bytes.
> + * Return strlen on success and < 0 on error.
>   */
> -static int bpf_obj_name_cpy(char *dst, const char *src)
> +int bpf_obj_name_cpy(char *dst, const char *src, unsigned int size)
>  {
> -       const char *end = src + BPF_OBJ_NAME_LEN;
> +       const char *end = src + size;
>
> -       memset(dst, 0, BPF_OBJ_NAME_LEN);
> +       memset(dst, 0, size);
>         /* Copy all isalnum(), '_' and '.' chars. */
>         while (src < end && *src) {
>                 if (!isalnum(*src) &&
> @@ -712,11 +712,11 @@ static int bpf_obj_name_cpy(char *dst, const char *src)
>                 *dst++ = *src++;
>         }
>
> -       /* No '\0' found in BPF_OBJ_NAME_LEN number of bytes */
> +       /* No '\0' found in "size" number of bytes */
>         if (src == end)
>                 return -EINVAL;
>
> -       return 0;
> +       return src - (end - size);

it's a rather convoluted way of writing (src - orig_src), maybe just
remember original src?

Either way not a big deal:

Acked-by: Andrii Nakryiko <andriin@...com>

>  }
>

[...]