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

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
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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

s/interconveritility/interconvertibility/ ?

quoted

  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.

quoted

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