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").
...
>
> 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 <aob...@pivotal.io>
> Signed-off-by: Mauricio Faria de Oliveira <m...@canonical.com>
> ---
> 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.
