Re: [PATCH v17 11/12] selftests/bpf: Add test for bpf_verify_pkcs7_signature() kfunc

[Yonghong Song]

On 9/15/22 9:11 AM, KP Singh wrote:
> On Fri, Sep 9, 2022 at 1:10 PM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> [...]
>
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > new file mode 100644
> > index 000000000000..4ceab545d99a
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_verify_pkcs7_sig.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > + */
> > +
> > +#include "vmlinux.h"
> > +#include <errno.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +#define MAX_DATA_SIZE (1024 * 1024)
> > +#define MAX_SIG_SIZE 1024
> > +
> > +typedef __u8 u8;
> > +typedef __u16 u16;
> > +typedef __u32 u32;
> > +typedef __u64 u64;
>
> I think you can avoid this and just use u32 and u64 directly.

For consistency of typical bpf programs and bpf.h, using __u{8,16,32,64}
should be okay.

> > +
> > +struct bpf_dynptr {
> > +       __u64 :64;
> > +       __u64 :64;
> > +} __attribute__((aligned(8)));
> > +
>
> I think you are doing this because including the uapi headers causes
> type conflicts.
> This does happen quite often. What do other folks think about doing
> something like
>
> #define DYNPTR(x) ((void *)x)
>
> It seems like this will be an issue anytime we use the helpers with
> vmlinux.h and users
> will always have to define this type in their tests.

We can use BTF_TYPE_EMIT macro to emit bpf_dynptr type to vmlinux dwarf 
and then to vmlinux BTF and vmlinux.h. For example, with below change,

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 41aeaf3862ec..bbdc53fec625 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1400,6 +1400,7 @@ static const struct bpf_func_proto 
bpf_kptr_xchg_proto = {

 static bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
 {
+       BTF_TYPE_EMIT(struct bpf_dynptr);
        return ptr->size & DYNPTR_RDONLY_BIT;
 }

in vmlinux.h, we will have

struct bpf_dynptr {
        long: 64;
        long: 64;};

> - KP
>
> > +extern struct bpf_key *bpf_lookup_user_key(__u32 serial, __u64 flags) __ksym;
> > +extern struct bpf_key *bpf_lookup_system_key(__u64 id) __ksym;
> > +extern void bpf_key_put(struct bpf_key *key) __ksym;
> > +extern int bpf_verify_pkcs7_signature(struct bpf_dynptr *data_ptr,
> > +                                     struct bpf_dynptr *sig_ptr,
> > +                                     struct bpf_key *trusted_keyring) __ksym;
> > +
> > +u32 monitored_pid;
> > +u32 user_keyring_serial;
> > +u64 system_keyring_id;
> > +
> > +struct data {
> > +       u8 data[MAX_DATA_SIZE];
> > +       u32 data_len;
> > +       u8 sig[MAX_SIG_SIZE];
> > +       u32 sig_len;
> > +};
> > +
> > +struct {
> > +       __uint(type, BPF_MAP_TYPE_ARRAY);
> > +       __uint(max_entries, 1);
> > +       __type(key, __u32);
> > +       __type(value, struct data);
> > +} data_input SEC(".maps");
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("lsm.s/bpf")
> > +int BPF_PROG(bpf, int cmd, union bpf_attr *attr, unsigned int size)
> > +{
> > +       struct bpf_dynptr data_ptr, sig_ptr;
> > +       struct data *data_val;
> > +       struct bpf_key *trusted_keyring;
> > +       u32 pid;
> > +       u64 value;
> > +       int ret, zero = 0;
> > +
> > +       pid = bpf_get_current_pid_tgid() >> 32;
> > +       if (pid != monitored_pid)
> > +               return 0;
> > +
> > +       data_val = bpf_map_lookup_elem(&data_input, &zero);
> > +       if (!data_val)
> > +               return 0;
> > +
> > +       bpf_probe_read(&value, sizeof(value), &attr->value);
> > +
> > +       bpf_copy_from_user(data_val, sizeof(struct data),
>
> [...]
>
> > --
> > 2.25.1