On 10/7/18 4:36 AM, Christian Brauner wrote:

quoted

+	if (cb->strict_check) {
+		struct ifinfomsg *ifm;
 
-		extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
-					  IFLA_EXT_MASK);
-		if (extfilt) {
-			if (nla_len(extfilt) < sizeof(filter_mask))
-				return -EINVAL;
+		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
+			NL_SET_ERR_MSG(extack, "Invalid header");
+			return -EINVAL;
+		}
+
+		ifm = nlmsg_data(nlh);
+		if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags ||
+		    ifm->ifi_change || ifm->ifi_index) {
+			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
+			return -EINVAL;
+		}
+	}
 
-			filter_mask = nla_get_u32(extfilt);
+	err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
+			  ifla_policy, extack);
+	if (err < 0) {
+		if (cb->strict_check)
+			return -EINVAL;
+		goto walk_entries;
+	}

What's the point of moving this out of the
if (cb->strict_check) {} branch above? This looks like it would cause
the same parse warnings that we're trying to get rid of in inet{4,6}
dumps.

Link messages don't have the problem in general because they use
ifinfomsg as the header - which is the one abused for other message
types. That said ...

Seems to make more sense to make the nlmsg_parse() itself conditional as
well unless I'm lacking context.

... I now have nlmsg_parse and nlmsg_parse_strict.