netdev - Re: [PATCH v2] libbpf: add request buffer type for netlink messages

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:   Wed, 16 Jun 2021 16:18:08 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] libbpf: add request buffer type for netlink messages

On Wed, Jun 16, 2021 at 10:04 AM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> Coverity complains about OOB writes to nlmsghdr. There is no OOB as we
> write to the trailing buffer, but static analyzers and compilers may
> rightfully be confused as the nlmsghdr pointer has subobject provenance
> (and hence subobject bounds).
>
> Remedy this by using an explicit request structure, but we also need to
> start the buffer in case of ifinfomsg without any padding. The alignment
> on netlink wire protocol is 4 byte boundary, so we just insert explicit

struct ifinfomsg has unsigned field in it, which makes it
automatically 4-byte aligned because the struct is not packed. Do we
really need that _pad[4] thing?.. Even if we do, I'm still not sure
how it helps with alignment... If anything, explicit
__attribute__((aligned(4))) would be better.

> 4 byte buffer to avoid compilers throwing off on read and write from/to
> padding.
>
> Also switch nh_tail (renamed to req_tail) to cast req * to char * so

it probably should use (void *) everywhere, instead of (char *), but I
see that existing code is using char * exclusively, so it's probably
for another patch

> that it can be understood as arithmetic on pointer to the representation
> array (hence having same bound as request structure), which should
> further appease analyzers.
>
> As a bonus, callers don't have to pass sizeof(req) all the time now, as
> size is implicitly obtained using the pointer. While at it, also reduce
> the size of attribute buffer to 128 bytes (132 for ifinfomsg using
> functions due to the need to align buffer after it).

Sorry if it's a stupid question, but why it's safe to reduce the
buffer size from 128 to 256?

>
> Summary of problem:
>   Even though C standard allows interconveritility of pointer to first

s/interconveritility/interconvertibility/ ?

>   member and pointer to struct, for the purposes of alias analysis it
>   would still consider the first as having pointer value "pointer to T"
>   where T is type of first member hence having subobject bounds,
>   allowing analyzers within reason to complain when object is accessed
>   beyond the size of pointed to object.
>
>   The only exception to this rule may be when a char * is formed to a
>   member subobject. It is not possible for the compiler to be able to
>   tell the intent of the programmer that it is a pointer to member
>   object or the underlying representation array of the containing
>   object, so such diagnosis is supressed.

typo: suppressed

>
> Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> ---
> Changelog:
> v1 -> v2:
>  * Add short summary instead of links about the underlying issue (Daniel)
> ---
>  tools/lib/bpf/netlink.c | 107 +++++++++++++++-------------------------
>  tools/lib/bpf/nlattr.h  |  37 +++++++++-----
>  2 files changed, 65 insertions(+), 79 deletions(-)
>

[...]