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

On Wed, Jun 16, 2021 at 10:04 AM Kumar Kartikeya Dwivedi
[off-list ref] 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(-)

[...]