[v2] libbpf: add request buffer type for netlink messages

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
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
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).

Summary of problem:
  Even though C standard allows interconveritility of pointer to first
  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.

Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.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(-)

--
2.31.1

On Wed, Jun 16, 2021 at 10:04 AM Kumar Kartikeya Dwivedi
<memxor@gmail.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@gmail.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(-)
>

[...]

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@gmail.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.

> > 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@gmail.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

On Wed, Jun 16, 2021 at 4:55 PM Kumar Kartikeya Dwivedi
<memxor@gmail.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@gmail.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@gmail.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