netdev - Re: [PATCH bpf-next v6 5/6] ipv6: sr: Add seg6local action End.BPF

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] [day] [month] [year] [list]

Message-ID: <CAKSCvkRPoxzy1vQ_Qx+TGo6586xmMhYVj9pAxK8vXYXZz1n-8w@mail.gmail.com>
Date:   Sun, 20 May 2018 12:21:07 +0100
From:   Mathieu Xhonneux <m.xhonneux@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, David Lebrun <dlebrun@...gle.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH bpf-next v6 5/6] ipv6: sr: Add seg6local action End.BPF

2018-05-18 21:24 GMT+01:00 Daniel Borkmann <daniel@...earbox.net>:
>> +#define MAX_PROG_NAME 256
>> +static const struct nla_policy bpf_prog_policy[LWT_BPF_PROG_MAX + 1] = {
>> +     [LWT_BPF_PROG_FD]   = { .type = NLA_U32, },
>
> From UAPI point of view, I wouldn't name it LWT_BPF_PROG_FD but rather something like
> LWT_BPF_PROG for example. That way, the setup can contain the fd number, but on the
> dump you can put the prog->aux->id in there so that prog lookup can be done again.

Good idea.

>> +     [LWT_BPF_PROG_NAME] = { .type = NLA_NUL_STRING,
>> +                             .len = MAX_PROG_NAME },
>> +};
>> +
>> +static int parse_nla_bpf(struct nlattr **attrs, struct seg6_local_lwt *slwt)
>> +{
>> +     struct nlattr *tb[LWT_BPF_PROG_MAX + 1];
>> +     struct bpf_prog *p;
>> +     int ret;
>> +     u32 fd;
>> +
>> +     ret = nla_parse_nested(tb, LWT_BPF_PROG_MAX, attrs[SEG6_LOCAL_BPF],
>> +                            bpf_prog_policy, NULL);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     if (!tb[LWT_BPF_PROG_FD] || !tb[LWT_BPF_PROG_NAME])
>> +             return -EINVAL;
>> +
>> +     slwt->bpf.name = nla_memdup(tb[LWT_BPF_PROG_NAME], GFP_KERNEL);
>> +     if (!slwt->bpf.name)
>> +             return -ENOMEM;
>> +
>> +     fd = nla_get_u32(tb[LWT_BPF_PROG_FD]);
>> +     p = bpf_prog_get_type(fd, BPF_PROG_TYPE_LWT_SEG6LOCAL);
>> +     if (IS_ERR(p))
>> +             return PTR_ERR(p);
>
> Here in the above error path is definitely a bug in that you don't free the
> prior allocated slwt->bpf.name from nla_memdup().

Indeed, I took this part from the lwt bpf infrastructure. I sent a
patch to fix it there too.

> Also when you destroy the struct seg6_local_lwt object, what I'm not getting
> is where you drop the prog reference again and free slwt->bpf.name there?

I totally missed this. Sending a v7 with all of this fixed.

Thanks for your comments !