Hey Kumar,
took in first two already, just few small nits in here, but overall looks
good to me.
On 6/12/21 4:35 AM, Kumar Kartikeya Dwivedi 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
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).
More info/discussion on why this was a problem in these links:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2294.htm#provenance-and-subobjects-container-of-casts-1
https://twitter.com/rep_stosq_void/status/1298581367442333696
Would be good if you could provide a small summary instead of external
links to twitter, so that this is ideally self-contained and doesn't
get lost from the log.
CID: 322807
CID: 322806
CID: 141815
CIDs are not official commit msg tags, and given this is just a coverity
false positive on top of that, I don't think we need them here.
Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
---
tools/lib/bpf/netlink.c | 107 +++++++++++++++-------------------------
tools/lib/bpf/nlattr.h | 37 +++++++++-----
2 files changed, 65 insertions(+), 79 deletions(-)
[...]
diff --git a/tools/lib/bpf/nlattr.h b/tools/lib/bpf/nlattr.h
index 3c780ab6d022..cc59f9c02d88 100644
--- a/tools/lib/bpf/nlattr.h
+++ b/tools/lib/bpf/nlattr.h
@@ -13,6 +13,7 @@
#include <string.h>
#include <errno.h>
#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
/* avoid multiple definition of netlink features */
#define __LINUX_NETLINK_H
@@ -52,6 +53,18 @@ struct libbpf_nla_policy {
uint16_t maxlen;
};
+struct netlink_request {
nit: Could we either name it struct libbpf_nla_req or just struct nlreq ...
to better fit the naming conventions of nlattr.h and not create yet a new
variant? Either is okay with me..
+ struct nlmsghdr nh;
+ union {
+ struct {
+ struct ifinfomsg ifinfo;
+ char _pad[4];
+ };
+ struct tcmsg tc;
+ };
+ char buf[128];
+};
+
/**
* @ingroup attr
* Iterate over a stream of attributes
@@ -111,44 +124,44 @@ static inline struct nlattr *nla_data(struct nlattr *nla)
return (struct nlattr *)((char *)nla + NLA_HDRLEN);
}
-static inline struct nlattr *nh_tail(struct nlmsghdr *nh)
+static inline struct nlattr *req_tail(struct netlink_request *req)
{
- return (struct nlattr *)((char *)nh + NLMSG_ALIGN(nh->nlmsg_len));
+ return (struct nlattr *)((char *)req + NLMSG_ALIGN(req->nh.nlmsg_len));
}
[...]
Thanks,
Daniel
