Date:   Wed, 16 Jun 2021 17:37:02 -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 4:55 PM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> On Thu, Jun 17, 2021 at 04:48:08AM IST, Andrii Nakryiko wrote:
> > 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.
> >
>
> What I meant was that reusing the same struct for both means that the trailing
> buffer where attributes are added starts right after struct tcmsg/struct
> ifinfomsg. Since tcmsg is 20 bytes, ifinfomsg is 16. I didn't want it to trigger
> if it ends up tracking the active member of the union (or effective type). Poor
> wording I guess. Everything is aligned properly, just wanted to explain why
> _pad[4] is there.

How

struct libbpf_nla_req {
       struct nlmsghdr nh;
       union {
               struct {
                       struct ifinfomsg ifinfo;
                       char _pad[4];
               };
               struct tcmsg tc;
       };
       char buf[128];
};

has different memory layout from just

struct libbpf_nla_req {
       struct nlmsghdr nh;
       union {
               struct ifinfomsg ifinfo;
               struct tcmsg tc;
       };
       char buf[128];
};


That _pad[4] just adds to confusion, IMO. Just trust the language and
its handling of union?..


>
> > > 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
> >
>
> I'll fix it in the resend.
>
> > > 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?
> >
>
> We just need something big enough, we already check the size everytime we add an
> attribute to make sure we don't run out of space. It was a remnant from previous
> versions where a lot of attributes were added. They're pretty limited now so I
> just changed to a small safe value that works fine for both.
>
> > >
> > > 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
> >
>
> Thanks.
>
> > >
> > > 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(-)
> > >
> >
> > [...]
>
> --
> Kartikeya

Powered by blists - more mailing lists