Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter family headers
From: David Ahern <hidden>
Date: 2018-10-01 07:41:33
On 9/28/18 1:35 PM, Mauricio Faria de Oliveira wrote:
Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
which is not always true. For example, 'struct ndmsg' is used by iproute2
as well (in the 'ip neigh' command).
The problem is, the function bails out early if nlmsg_parse() fails, which
does occur for iproute2 usage of 'struct ndmsg' because the payload length
is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
This breaks backward compatibility with userspace (different response) and
is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from
rtnl_fdb_dump() on parse error")....
quoted hunk ↗ jump to hunk
Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error") Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl") Reported-by: Aidan Obley <redacted> Signed-off-by: Mauricio Faria de Oliveira <redacted> --- P.S.: this may be 'net', but labeling as 'net-next' for possible relation to recent thread [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request net/core/rtnetlink.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 60c928894a78..9695a27cc9b9 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c@@ -3744,16 +3744,17 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb) int err = 0; int fidx = 0; - err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, - IFLA_MAX, ifla_policy, NULL); - if (err < 0) { - return -EINVAL; - } else if (err == 0) { + /* The family header may _not_ be struct ifinfomsg + * (e.g., struct ndmsg). Usage of the ifm pointer + * must check payload length (e.g., nlmsg_parse()). + */ + if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb, + IFLA_MAX, ifla_policy, NULL) == 0) { if (tb[IFLA_MASTER]) br_idx = nla_get_u32(tb[IFLA_MASTER]); - } - brport_idx = ifm->ifi_index; + brport_idx = ifm->ifi_index; + } if (br_idx) { br_dev = __dev_get_by_index(net, br_idx);
I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
the header if any kernel side filtering is to be done. As for the change
above, I suggest something like this:
/* if header struct is ndmsg, no attributes can be appended */
if (nlmsg_len(nlh) != sizeof(struct ndmsg)) {
current ifinfomsg based code
}
We certainly do not want to ignore parse failures.